[PWA-1658] Move graphql-cli-validate-magento-pwa-queries to @magento scope #3198

tjwiebell · 2021-05-27T18:07:16Z

Description

As part of https://jira.corp.magento.com/browse/PWA-1602 we realized that all our packages except graphql-cli-validate-magento-pwa-queries are under the @magento custom scope.

Using custom scopes safe guards us against supply chain attacks. So as part of this PR, lets move graphql-cli-validate-magento-pwa-queries under magento scope and deprecate the old package.

Acceptance Criteria

Publish the package to the new namespace / scope
Submit a PR pointing any existing code in PWA Studio to the new package, to be released in the next PWA Studio release
Communicate the upcoming deprecation to stakeholders, including community
(Ideally after next release) post a deprecation notice / deprecate the existing package that is not in the magento scope

Proposed Solution

graphql-cli@3 requires plugin names begin with graphql-cli-, so re-naming was not going to be possible. It seemed achievable that upgrading to graphql-cli@4 would mean we could leverage their new plugin framework, but from my discovery the naming requirement now meant you had to be in the @graphql-cli namespace. Before digging too much into that solution, I instead focused on what it would take to get graphql validate working in the project.

The solution proposed has some flaws, and is a tiny bit less feature rich, but accomplishes the same task. It also gives us the opportunity to start leveraging some cooler features of graphql-cli, like type generation, or change how we approach operation creation.

Related Issue

[PWA-1658] Move graphql-cli-validate-magento-pwa-queries to @magento scope

Related PRs

https://github.com/magento-commerce/pwa-studio-cicd/pull/73

Acceptance

Verification Stakeholders

Specification

Verification Steps

yarn validate-queries - verify no errors returned (expected to see lots of deprecation warnings)
yarn lint - verify no errors

Screenshots / Screen Captures (if appropriate)

Checklist

I have added tests to cover my changes, if necessary.
I have added translations for new strings, if necessary.
I have updated the documentation accordingly, if necessary.

PWAStudioBot · 2021-05-27T18:17:47Z

	Messages
📖	Associated JIRA tickets: PWA-1602.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against 5b7fd9c

…h old version

sirugh · 2021-05-28T18:26:34Z

packages/venia-ui/lib/components/CartPage/PriceAdjustments/GiftOptions/giftOptions.gql.js

-            gift_message
+            include_gift_receipt @client
+            include_printed_card @client
+            local_gift_message @client


Why the rename here?

gift_message is a field in Cart now, which didn't have the same type as our local defined one; validator threw an error. I don't think it did at the time of writing, so this ideally this should be re-factored to use the real fields, now that they're available. Could be a good first task for community, happy to write that up if you agree.

Yea I think the original intent behind the message was to actually hook this up :) As is, it doesn't actually do anything which might be surprising.

sirugh · 2021-06-01T14:48:27Z

Don't forget to provide the version label and, if breaking, to include in the PR description what breaks.

- Update scaffold template to remove old commands

tjwiebell · 2021-06-02T18:39:14Z

.eslintrc.js

+            },
+            plugins: ['@graphql-eslint'],
+            rules: {
+                '@graphql-eslint/known-directives': 'error',


I've only added a small subset of rules here from packages/graphql-cli-validate-magento-pwa-queries/lib/index.js. When I added all the validation rules listed in that file, the linter blew up with errors, so I suspect those validation rules weren't working as expected. Trying to keep this scope minimal to just feature parity with what we had before, which I why I selected this subset of rules that were working.

sirugh

Good work, only a few comments. And I'm getting some failures locally around wishlistFragment (of which you are aware).

sirugh · 2021-06-03T14:18:43Z

.eslintrc.js

+    },
+    overrides: [
+        {
+            files: ['**/*.gql.js'],


We have some files named *.gql.ce.js and .gql.ee.js which I believe will get missed by this rule.

Updated pattern in b9de346

sirugh · 2021-06-03T14:26:38Z

packages/venia-concept/.graphqlconfig

@@ -1,25 +1,11 @@
 {
    "projects": {
        "venia": {
-            "schemaPath": "lastCachedGraphQLSchema.json",
+            "schemaPath": "merged-schema.v3.json",


Config structure for graphql-config@3

sirugh · 2021-06-03T14:26:50Z

graphql.config.js

+    extensions: {
+        codegen: {
+            generates: {
+                './merged-schema.v4.json': {


New config structure for graphql-config@4. This codegen is actually broken until we fix a lot of other naming collisions, so I may remove this until we can resolve those.

Removed codegen section in 52ed88d

sirugh · 2021-06-03T14:28:04Z

packages/venia-concept/client-schema.graphql

@@ -0,0 +1,19 @@
+extend type Query {


Mind throwing a comment block above explaining who/what this file is for?

For sure 👍 intend to collocate these closer to where they are defined, similar to what I did with the checkmo extension.

sirugh · 2021-06-03T14:29:07Z

packages/venia-concept/package.json

    "lint": "eslint --ignore-path .gitignore 'src/**/{*.js,package.json}'",
    "prettier": "prettier --loglevel warn --ignore-path .gitignore 'src/**/*.@(css|graphql|js)' '*.js'",
    "prettier:check": "yarn run -s prettier -- --check",
    "prettier:fix": "yarn run -s prettier -- --write",
    "start": "buildpack serve .",
    "start:debug": "node --inspect-brk ./node_modules/.bin/webpack-dev-server --progress --color --env.mode development",
    "test": "yarn run -s prettier:check && yarn run -s lint && jest",
-    "validate-queries": "yarn run download-schema && graphql validate-magento-pwa-queries --project venia",


yarn build:dev uses this command still - will removing it cause breaks?

Also how will folks validate their queries in their scaffolded apps without this script/

This is the fileglob that was previously in .graphqlconfig that was used by the plugin. validate-queries didn't actually do anything in scaffolded apps 😬 those packages would be in node_modules on a scaffold. The feature parity I was aiming for was just to restore validation in the monorepo.

"filesGlob": "../{peregrine,venia-ui,venia-concept}/{lib,src}/**/*.{js,graphql,gql}"

edit - But yes, this would cause breakage in build:dev. Will remove it.

Removed from build:dev in b9de346

validate-queries didn't actually do anything in scaffolded apps

I am not 100% sure that validate-queries is not doing nothing on scaffolded apps. You can test this by pointing your MAGENTO_BACKEND_URL at an invalid server. If you then run yarn watch prior to this PR you will get something like

message: 'invalid json response body at https://magento2.foomanstore.test/graphql reason: Unexpected token p in JSON at position 4', type: 'invalid-json'

So while we may not validate any queries at least grapqhl schema needs to generate successfully (funnily we have had a case where the back-end schema did not work but all other queries did).

@fooman - This was a tiny reason why I left the graphql-config@2 or .graphqlconfig around in venia-concept, but for a different reason; so IDEs would still be able to fetch schemas to provide in editor validation (eg. I use this webstorm plugin, which doesn't yet support v3).

Ideal state is that validation works in scaffolded apps the same as the monorepo, and the state in this PR is no validation. We do introspection at build time for fragment matching, so I'm not sure a validate-queries command would indicate much, but happy to hear any ideas for a middle ground state that isn't just a removal of this command. Is this more about the nice messaging the previous plugin provided?

Thanks for getting back to me. Can confirm that latest scaffolded is not working well with validate-queries (it may have done so in the past as did see a lastCachedGraphQLSchema.json at some point). So I guess the suggestion here is reasonable to fix it first for the monorepo and then hopefully we can get this to work with scaffolded apps as the next step.

With quite a bit of tinkering I am now able to run validate-queries locally in a scaffolded apps.

In rough steps (I am not running the same apollo version which may be the reason for step 1):

force a graphql resolution

edit .graphqlconfig to find the files under node_modules with "filesGlob": "./node_modules/@magento/{peregrine,venia-ui,venia-concept}/{lib,src}/**/*.{graphql,gql,gql.js}",

overcome default node_module exclusion by adding ignorePattern: '!node_modules/*' to linterConfiguration
to node_modules/graphql-cli-validate-magento-pwa-queries/lib/index.js

Profit? [bug]: Query uses non existing WishlistVisibilityEnum in CE #3217

@fooman
Given how tech debt is usually treated I'll see if I can make that happen in this scope. PR welcome, otherwise I'll work on it tomorrow.

sirugh · 2021-06-03T14:30:06Z

packages/venia-ui/lib/components/CheckoutPage/PaymentInformation/paymentInformation.gql.js

 export const GET_PAYMENT_NONCE = gql`
    query getPaymentNonce($cartId: String!) {
-        cart(cart_id: $cartId) @client {


In many of the queries we move @client to the local such as here for paymentNonce. Does this result in many more queries than were previously sent?

If the fetchPolicy was cache-and-network, I could maybe see this hitting the network now. I was getting a validation error with these on the operation instead of the fields, but now that those directives are merged into the schema, I'm not getting those anymore. I think this can safely be backed out.

Reverted these directive changes in b9de346

dpatil-magento · 2021-06-08T20:19:52Z

QA Approved

tjwiebell added 2 commits May 24, 2021 11:20

Some preliminary work to get introspection working in v4

fa7a5b0

Get new cli validation passing following the current eslint rules

f8a6f42

m2-community-project bot added the Progress: ready for review label May 27, 2021

PWAStudioBot added pkg:graphql-cli-validate-magento-pwa-queries pkg:peregrine pkg:venia-concept pkg:venia-ui labels May 27, 2021

Minor cleanup

063b1bc

tjwiebell force-pushed the tommy/rename-validator branch from 047411e to 063b1bc Compare May 27, 2021 18:19

Remove package from workspaces so it doesn't pollute node_modules wit…

f005420

…h old version

sirugh reviewed May 28, 2021

View reviewed changes

m2-community-project bot assigned sirugh May 28, 2021

m2-community-project bot added Progress: review and removed Progress: ready for review labels May 28, 2021

tjwiebell added 2 commits June 2, 2021 13:01

Restore additional graphql validation through linting rules

2b15e9a

- Re-add package dependencies that were being hidden by hoisting

99068e6

- Update scaffold template to remove old commands

PWAStudioBot added pkg:extensions pkg:pagebuilder pkg:pwa-buildpack labels Jun 2, 2021

tjwiebell commented Jun 2, 2021

View reviewed changes

tjwiebell added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Jun 2, 2021

tjwiebell and others added 3 commits June 2, 2021 14:30

Test adding additional files to copy command

74243f0

Back out change that didn't work

4252a9a

Merge branch 'develop' into tommy/rename-validator

5f025c7

sirugh requested changes Jun 3, 2021

View reviewed changes

m2-community-project bot added Progress: changes requested and removed Progress: review labels Jun 3, 2021

m2-community-project bot added Progress: review and removed Progress: changes requested labels Jun 3, 2021

tjwiebell added 3 commits June 3, 2021 11:39

Address PR feedback and resolve validation errors with name collisions.

b9de346

Fix missed @client directive

0f56922

Remove codegen section until we can resolve name conflicts

52ed88d

m2-community-project bot assigned fooman Jun 3, 2021

fooman removed their assignment Jun 3, 2021

m2-community-project bot assigned fooman Jun 3, 2021

fooman mentioned this pull request Jun 3, 2021

[bug]: Query uses non existing WishlistVisibilityEnum in CE #3217

Closed

8 tasks

fooman removed their assignment Jun 3, 2021

Colocate client schemas with components

f94d9dc

sirugh approved these changes Jun 7, 2021

View reviewed changes

m2-community-project bot added Progress: approved and removed Progress: review labels Jun 7, 2021

Merge branch 'develop' into tommy/rename-validator

5b7fd9c

dpatil-magento merged commit adae9d8 into develop Jun 8, 2021

dpatil-magento deleted the tommy/rename-validator branch June 8, 2021 20:20

m2-community-project bot added Progress: done and removed Progress: approved labels Jun 8, 2021

sirugh mentioned this pull request Jun 9, 2021

[FEATURE] #3102 Create a new Target for EditModal and Summary #3103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PWA-1658] Move graphql-cli-validate-magento-pwa-queries to @magento scope #3198

[PWA-1658] Move graphql-cli-validate-magento-pwa-queries to @magento scope #3198

tjwiebell commented May 27, 2021 •

edited

Loading

PWAStudioBot commented May 27, 2021 •

edited

Loading

sirugh May 28, 2021

tjwiebell May 28, 2021

sirugh May 28, 2021

sirugh commented Jun 1, 2021

tjwiebell Jun 2, 2021

sirugh left a comment

sirugh Jun 3, 2021

tjwiebell Jun 3, 2021

sirugh Jun 3, 2021

tjwiebell Jun 3, 2021

sirugh Jun 3, 2021

tjwiebell Jun 3, 2021

tjwiebell Jun 3, 2021

sirugh Jun 3, 2021

tjwiebell Jun 3, 2021

sirugh Jun 3, 2021

tjwiebell Jun 3, 2021 •

edited

Loading

tjwiebell Jun 3, 2021

fooman Jun 3, 2021

tjwiebell Jun 3, 2021

fooman Jun 3, 2021

fooman Jun 3, 2021

tjwiebell Jun 4, 2021

sirugh Jun 3, 2021

tjwiebell Jun 3, 2021

tjwiebell Jun 3, 2021

dpatil-magento commented Jun 8, 2021

[PWA-1658] Move graphql-cli-validate-magento-pwa-queries to @magento scope #3198

[PWA-1658] Move graphql-cli-validate-magento-pwa-queries to @magento scope #3198

Conversation

tjwiebell commented May 27, 2021 • edited Loading

Description

Proposed Solution

Related Issue

Related PRs

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirugh commented Jun 1, 2021

Choose a reason for hiding this comment

sirugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjwiebell Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpatil-magento commented Jun 8, 2021

tjwiebell commented May 27, 2021 •

edited

Loading

PWAStudioBot commented May 27, 2021 •

edited

Loading

tjwiebell Jun 3, 2021 •

edited

Loading