-
Notifications
You must be signed in to change notification settings - Fork 683
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
Validate Queries Plugin #1004
Validate Queries Plugin #1004
Conversation
This pull request is automatically deployed with Now. |
Adding the |
The Remaining steps for this PR:
|
packages/upward-spec/package.json
Outdated
@@ -28,7 +28,7 @@ | |||
"apollo-server": "~2.3.1", | |||
"chalk": "~2.4.2", | |||
"csv-parse": "~4.3.0", | |||
"graphql": "~14.0.2", | |||
"graphql": "~14.1.1", |
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 had to bump these versions due to an "incompatibility" bug where multiple instances of the graphql
dependency were installed and the plugin didn't know which one to use.
Right now these are manually kept in sync. I would very much welcome a better way to do this, it seems to me like the version of graphql
in upward-js
and upward-spec
should be completely independent.
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.
Should this just be a peer dep of the consuming project? IMO the more peer deps we delegate to the final consumer, the better, since that's a great way to prevent duplicate packages in a final bundle.
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.
Do upward-js
and upward-spec
even depend on the graphql
dependency? I see a single require('graphql-tag') here but other than that I don't see anything.
I also have next to no familiarity with these codebases though 😛
One last item that I'd appreciate feedback on: Should My gut is that |
@supernova-at I don't think the queries in Peregrine should be in there. @jimbo could convince me otherwise, but I would rather Peregrine be pretty platform-agnostic. Its |
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 looks great. Since we eventually want to make a Buildpack CLI that does this Magento-specific stuff (see my comment below), we'll have to come revisit this no matter what. Because of that, I think it's safe to ignore some of the modularity ideas we discussed, and instead just make this plugin a Magento PWA Studio specific graphql-cli
plugin, so you can combine the plugin code and the query report code.
packages/venia-concept/package.json
Outdated
"prepublishOnly": "yarn run build", | ||
"start": "node server.js", | ||
"start:debug": "node --inspect-brk ./node_modules/.bin/webpack-dev-server --progress --color --env.mode development", | ||
"stats": "webpack-bundle-analyzer dist/build-stats.json", | ||
"storybook": "start-storybook -p 9001 -c .storybook", | ||
"storybook:build": "build-storybook -c .storybook -o storybook-dist", | ||
"validate-queries": "node ./validate-queries.js", | ||
"validate-queries": "yarn run download-schema && graphql validate-queries --project venia --filesGlob \"src/**/*.{js,graphql,gql}\" --clients apollo literal && node ./summarize-validate-queries.js", |
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.
Eventually we'd want the same command to both:
- Confirm that the cached local schema still matches the remote schema
- Validate all the queries
I figure once we have a real @magento/pwa-buildpack
cli, like pbuild
, it would look a little more like:
"validate-queries": "pbuild verify-schema && graphql validate-queries | pbuild diagnose-queries"
Speaking of which, can you try making the CLI flags optional, and having them fall back to extension configuration in .graphlconfig
?
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 ended up just removing them and always pulling them from .graphqlconfig
.
ac0fd82
to
ed84d16
Compare
Creates a compatibility definition file and script to generate markdown from it Creates a graphql-cli extension to validate a project's queries Created a new project / repository called graphql-cli-validate-queries that - based on configuration - goes through all of a project's GraphQL queries and runs them against an endpoint's schema. Updates venia-concept to use this new extension and the graphql-cli tool in general. Resolves #952. Fixes the format of the graphql/template-strings eslint rule Adds script to dev-docs to auto-generate the magento compatibility table Adds a README to the graphql-cli-validate-queries project Adds unit tests to graphql-cli-validate-queries plugin. Installs and wires up Jest for unit testing the validate-queries plugin. Fixed typo mistakes. (#993) * Fixed typo mistakes * Fixed typo mistakes chore: remove duplicate padding property (#941) [BUGFIX] [ISSUE-958] Autocomplete renders loading component on clear (#961) * [BUGFIX] [ISSUE-958] Autocomplete renders loading component on clear Removes invalid TODO in graphql-cli-validate-queries/index.js Adds a JSDoc comment block to the getValidator function in graphql-cli-validate-queries Cleans up the devdoc draft for validate-queries plugin Adds a PWA-specific summarization to the end of the validate-queries command. Adds a new script summarize-validate-queries.js to venia-concept Runs this script at the end of the validate-queries script This script uses the compatibility table to clue developers to compatibility issues and how to resolve them Runs prettier Removes use of reserved word in summarize-validate-queries.js Create compatibility table page (#1016) * Fix auto-generated table markdown and re-order versions to list the latest version first * Incorporate build-compatibility table script into build process * Create a page to display the compatibility table * Update pwa-devdocs/src/technologies/magento-compatibility/index.md Co-Authored-By: jcalcaben <[email protected]> Updates the path to the compatibility table on the docs site Updates the wording around the reporting of version specifics and compatibility in the summarize-validate-queries script Runs prettier and updates summarize-validate-queries. Fixes dependencies and includes plugin in root jest test. The graphql-cli-validate-magento-pwa-queries plugins tests are now run as part of the root test command. This ensures that breaking tests in this project will fail the prepush check. Cleans up yarn.lock changes by selectively installing dependencies via yarn workspace add commands. Renames plugin to be more magento and pwa specific. Runs prettier Removes unneeded summary script file Updates the dev docs for the graphql-cli plugin
ed84d16
to
a156a48
Compare
I had to rebase / force push because it was insanity to merge I ended up rebasing, deleting |
…a-queries from the registry instead of the local file system
PR Updated:
|
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.
LGTM, this is a great addition. A good place to start generalizing these tools back into the graphql ecosystem.
() => ({ | ||
testEnvironment: 'node' | ||
}) | ||
) |
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 sticking with this convention we're doing.
* [graphql-cli](https://github.com/graphql-cli/graphql-cli) | ||
* [eslint-plugin-graphql](https://github.com/apollographql/eslint-plugin-graphql) | ||
* [graphql/no-deprecated-fields rule](https://github.com/apollographql/eslint-plugin-graphql#no-deprecated-fields-validation-rule) | ||
|
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 LOVE this documentation. Thank you for making 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 noticed a table formatting error, pushing commit now.
"endpoints": { | ||
"default": "${env:MAGENTO_BACKEND_URL}graphql" | ||
}, | ||
"validate-magento-pwa-queries": { |
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.
Excellent
@@ -45,7 +46,8 @@ | |||
"apollo-link-context": "^1.0.14", | |||
"apollo-link-http": "^1.5.11", | |||
"braintree-web-drop-in": "^1.16.0", | |||
"graphql": "^14.1.1", | |||
"graphql-cli": "^3.0.11", | |||
"graphql-cli-validate-magento-pwa-queries": "^1.0.0", |
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 is really a mouthful. 😆
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.
Lol yeah, it was much better as validate-queries
but then we made it magento / pwa specific 😝
@@ -0,0 +1,21 @@ | |||
MIT License | |||
|
|||
Copyright (c) 2019 Adobe Inc. |
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 an official thing?
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.
Published to NPM so I was just covering all the bases 🤷♂️
"schemaPath": "lastCachedGraphQLSchema.json", | ||
"extensions": { | ||
"endpoints": { | ||
"default": "${env:MAGENTO_BACKEND_URL}graphql" |
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.
Description
graphql-cli-validate-magento-pwa-queries
This PR creates a new
graphql-cli
plugin calledvalidate-magento-pwa-queries
.Venia depends on
graphql-cli
and this new plugin to do the work of validating the queries, replacing the need forvalidate-queries.js
inpackages/venia-concept/
itself.Venia's
validate-queries
script runs the plugin.Magento Compatibility Table
There is a new file called
magento-compatibility.js
at the root of PWA Studio. It currently looks like this:This file is intended to be the source of truth for which versions of PWA are compatible with which versions of Magento.
The file is currently used by a new script (
create-compatibility-table.js
) inpwa-devdocs
that creates a markdown table in its_includes
directory for inclusion on the docs site.It is also used by a new script (
summarize-validate-queries.js
) invenia-concept
that looks up the current version of PWA (based on theversion
field in the rootpackage.json
) and informs developers which versions of Magento are compatible.📝 This file should definitely be considered a "living document" and we should update it as appropriate during releases.
Related Issue
Closes #952 .
Motivation and Context
From the issue:
Verification
First,
yarn clean:all
andyarn install
to pick up the new dependencies.Testing
2.3.0
MAGENTO_BACKEND_URL
inenv
to https://release-dev-rxvv2iq-zddsyhrdimyra.us-4.magentosite.cloud/graphql.yarn validate-queries
.Testing
2.3.1
MAGENTO_BACKEND_URL
inenv
to https://release-dev-231-npzdaky-zddsyhrdimyra.us-4.magentosite.cloud/.yarn validate-queries
.How Has This Been Tested?
The verification steps above, and
yarn test
.Screenshots (if appropriate):
Proposed Labels for Change Type/Package
FEATURE
venia-concept
Checklist:
TODOs
Incorporate the downloading of the schema into the plugin itselfgraphql-cli-validate-queries
plugin to NPM and install it from there instead of using the local filesystemprocess.exit(1)
effectively?Followup
Open a PR to graphql-cli and getvalidate-queries
listed as a plugin on their main README.