-
Notifications
You must be signed in to change notification settings - Fork 23
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
Conversation
handle `apollo.config.json` as jsonc.
You can download the latest build of the extension for this PR here: 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
module.exports = { | ||
client: { | ||
tagName: "graphql", // highlight-line | ||
service: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually rendered out in the Marketplace, so I'm removing it
There was a problem hiding this 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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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.
<h3>Setting the Extension up for use with Apollo GraphOS</h3> | |
<h3>Configure extension for schemas published to Apollo GraphOS</h3> |
README.md
Outdated
<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" | ||
} | ||
} |
There was a problem hiding this comment.
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.,
- Obtain a personal API key.
- Edit the
apollo.config.json
to look like this... - 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this 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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log("setup"); |
Is this needed?
There was a problem hiding this comment.
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
There was a problem hiding this 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!
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.