Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change default recommended configuration file format to json. #209

Merged
merged 9 commits into from
Sep 18, 2024

Conversation

phryneas
Copy link
Member

This changes the default recommended configuration file format to .json.

I would prefer to make this yaml instead, but the configuration's JSON schema is natively supported by VSCode for .json files and needs an additional extension to be installed by the user to support .yaml files, so I believe going this way will result in less onboarding friction.

Copy link
Contributor

github-actions bot commented Sep 17, 2024

You can download the latest build of the extension for this PR here:
vscode-apollo-0.0.0-build-1726646915.pr-209.commit-96c069e.zip.

To install the extension, download the file, unzip it and install it in VS Code by selecting "Install from VSIX..." in the Extensions view.

Alternatively, run

code --install-extension vscode-apollo-0.0.0-build-1726646915.pr-209.commit-96c069e.vsix --force

from the command line.

For older builds, please see the edit history of this comment.

@@ -26,20 +26,31 @@ The Apollo GraphQL extension for VS Code brings an all-in-one tooling experience

<h2 id="getting-started">Getting started</h2>

For the VS Code plugin to know how to find the schema, it needs to be linked to either a published schema or a local one.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Meschreiber, could I ask you to take a quick look at these changes? Once we ship it here and the details tags are working, I would add the same changes to the corresponding Docs page.

I made these changes as we get a lot of reviews that complain that you need a GraphOS account, which is not true. It seems the alternative configurations for local service and local file were not prominent enough until now.

By making these sections collapsible, I'm trying to make all of them more discoverable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the ping here!

I didn't realize the README for this project had a lot of duplicated content with the docs page. Did we consider just linking to the docs page rather than maintaining both places?/Do you think the benefit of visibility (particularly in the VSC marketplace) outweighs the minor burden of maintaining in both places?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, definitely - especially the screenshots help a lot showcasing the features we have. I'm happy to keep both in sync :)

build.onResolve(
{ filter: /^jsonc-parser$/ },
async ({ path, ...options }) => {
return build.resolve("jsonc-parser/lib/esm/main.js", options);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This package offers ESM and UMD. If I import the UMD in VSCode (the default), it makes the extension crash - if I import the ESM in the tests, the tests crash.
This way, the tests get UMD and the extension bundles with the ESM import.

Comment on lines -199 to -202
module.exports = {
client: {
tagName: "graphql", // highlight-line
service: ...
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
This actually rendered out in the Marketplace, so I'm removing it

@phryneas phryneas requested a review from alessbell September 17, 2024 15:43
Copy link
Contributor

@Meschreiber Meschreiber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some structural suggestions and a question. Otherwise LGTM!

@@ -26,20 +26,31 @@ The Apollo GraphQL extension for VS Code brings an all-in-one tooling experience

<h2 id="getting-started">Getting started</h2>

For the VS Code plugin to know how to find the schema, it needs to be linked to either a published schema or a local one.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the ping here!

I didn't realize the README for this project had a lot of duplicated content with the docs page. Did we consider just linking to the docs page rather than maintaining both places?/Do you think the benefit of visibility (particularly in the VSC marketplace) outweighs the minor burden of maintaining in both places?

README.md Outdated
To get all the benefits of the VS Code experience, it's best to link the schema that is being developed against before installing the extension. The best way to do that is by [publishing a schema](https://www.apollographql.com/docs/graphos/delivery/publishing-schemas/) to the Apollo schema registry. After that's done:

1. Create an `apollo.config.js` file at the root of the project.
Alternatively, you can create a `cjs`, `mjs`, or `ts` file with the same configuration.
1. Create an `apollo.config.json` file at the root of the project.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this step is no longer necessary since it's mentioned above?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, and this is only a summary of what's coming next, so I'll delete the whole thing.

README.md Outdated

<details>
<summary>
<h3>Setting the Extension up for use with Apollo GraphOS</h3>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important that the title clearly states this is the "published" option.

Suggested change
<h3>Setting the Extension up for use with Apollo GraphOS</h3>
<h3>Configure extension for schemas published to Apollo GraphOS</h3>

README.md Outdated
Comment on lines 46 to 55
<h3 id="apollo-config">Setting up an Apollo config</h3>

For the VS Code plugin to know how to find the schema, it needs to be linked to either a published schema or a local one. To link a project to a published schema, edit the `apollo.config.js` file to look like this:
To link a project to a published schema, edit the `apollo.config.json` file to look like this:

```js
module.exports = {
client: {
service: "my-graphql-app",
},
};
```json
{
"client": {
"service": "graphos-graph-id"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this and the following <h3> and just have one continuous set of instructions. E.g.,

  1. Obtain a personal API key.
  2. Edit the apollo.config.json to look like this...
  3. To pull down your schema, create an .env file ...

@@ -50,7 +61,7 @@ See [additional configuration options](#additional-apollo-config-options).

<h3 id="api-key">Setting up the <code>.env</code> file</h3>

To authenticate with GraphOS Studio to pull down your schema, create a `.env` file in the same directory as the `apollo.config.js` file. This should be an untracked file (that is, don't commit it to Git).
To authenticate with GraphOS Studio to pull down your schema, create a `.env` file in the same directory as the `apollo.config.json` file. This should be an untracked file (that is, don't commit it to Git).

Then go to your [User Settings page](https://studio.apollographql.com/user-settings/api-keys?referrer=docs-content) in GraphOS Studio to create a new Personal API key.
Copy link
Contributor

@Meschreiber Meschreiber Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this different than the API key they needed to previously obtain?

Copy link
Member Author

@phryneas phryneas Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the exact same one. This whole thing is just a bit too repetitive 😅
Removed the earlier mention.

README.md Outdated

<details>
<summary>
<h3 id="local-schemas">Setting the Extension up for use with a local schema</h3>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<h3 id="local-schemas">Setting the Extension up for use with a local schema</h3>
<h3 id="local-schemas">Configure extension for local schemas</h3>

</details>

<details>
<summary>
Copy link
Contributor

@Meschreiber Meschreiber Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would include this under the same header (Configure extension for local schemas) and present the two options has <h4>s.

  • Use introspection of a locally running service
  • Use with a local schema file.

IMO, since these are two flavors of the "Configure extension for local schemas" they should be separate <h4>s within that collapsible section.

Copy link
Member Author

@phryneas phryneas Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People have been missing these options a lot, so I want to have both options as visible as the GraphOS one. It's really pulling down our marketplace ratings and leads to bad reviews from non-GraphOS users (which make up a big part of our userbase - we have 500k downloads).

Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spotted one stray log, but other than Maria's requests, I've got nothing else.

src/build.js Outdated
const resolvePlugin = {
name: "resolve",
setup(build) {
console.log("setup");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log("setup");

Is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not anymore, good catch :D

Copy link
Contributor

@Meschreiber Meschreiber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for restructuring, looks a lot clearer!

@phryneas phryneas merged commit fc73ed3 into main Sep 18, 2024
8 checks passed
@phryneas phryneas deleted the pr/default-config-json branch September 18, 2024 13:59
@github-actions github-actions bot mentioned this pull request Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants