[chore] Initial attempt at fixing validator for venia-ui #2095

sirugh · 2020-01-10T21:21:09Z

Description

yarn run validate-queries was incomplete. It only validated venia-concept which no longer has any any graphql. That's not to say it won't always, but we should at least have had the validator check venia-ui.

Note: I added the validator to pre-push but I also changed the errors to warnings so as to not kill the push. We can discuss in this PR.

Related Issue

Closes PWA-251

Acceptance

Verification Stakeholders

Ya'll.

Specification

Verification Steps

Run yarn run validate-queries. Result should display.

Screenshots / Screen Captures (if appropriate)

Checklist

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

PWAStudioBot · 2020-01-10T21:27:05Z

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	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).
📖	Associated JIRA tickets: PWA-251.

Generated by 🚫 dangerJS against 6997ae8

Signed-off-by: Stephen Rugh <rugh@adobe.com>

sirugh · 2020-01-24T22:48:05Z

package.json

@@ -90,7 +90,7 @@
  },
  "husky": {
    "hooks": {
-      "pre-push": "yarn run prettier:check && yarn run lint"
+      "pre-push": "yarn run prettier:check && yarn run lint && yarn run validate-queries"


This is debatable. Do we want to run this on push? I think so.

I mean, we don't currently. I'm okay with adding it since I always push with --no-verify anyway.

Let's ask the team

@dpatil-magento can we add this script to the CI checks?

https://jira.corp.magento.com/browse/PWA-341 was created -- team says NO to prepush :D

sirugh · 2020-01-24T22:48:44Z

packages/graphql-cli-validate-magento-pwa-queries/lib/index.js

@@ -67,7 +67,7 @@ async function validateQueries(context, argv) {

        // Validate our queries against that schema.
        context.spinner.start('Finding queries in files...');
-        const validator = getValidator({ clients, project });
+        const validator = getValidator({ clients, project, schemaPath });


Now the validator will run the schema stored in the path rather than doing a "magical" search for the file within the directory the script is running. Basically we can run this from venia-concept but still check files in the sibling package, venia-ui.

sirugh · 2020-01-24T22:50:58Z

packages/graphql-cli-validate-magento-pwa-queries/lib/index.js

    }));

-    const ruleDefinition = ['error', ...clientRules];
+    const ruleDefinition = ['warn', ...clientRules];


This is debatable. I know some folks think warnings are code smells. If we want to leave this as error we should then fix all the issues in the current graphql files (see the screenshot in the PR).

Yes, in this repo, we do prefer errors over warnings. That said, if you'd like to create an issue to fix all the warnings (and then flip it back from warn to error), that would be fine.

Signed-off-by: Stephen Rugh <rugh@adobe.com>

sirugh · 2020-01-24T22:53:47Z

packages/venia-concept/.graphqlconfig

@@ -8,7 +8,7 @@
                },
                "validate-magento-pwa-queries": {
                    "clients": ["apollo", "literal"],
-                    "filesGlob": "src/**/*.{js,graphql,gql}"
+                    "filesGlob": "../{venia-ui,venia-concept}/{lib,src}/**/**/*.{js,graphql,gql}"


I'm bad with patterns...do I need the extra /**?

I don't think you do.

sirugh · 2020-01-24T23:03:01Z

I will fix tests once we land on a consensus for the debatable bits.

jimbo · 2020-01-29T20:33:22Z

package.json

@@ -90,7 +90,7 @@
  },
  "husky": {
    "hooks": {
-      "pre-push": "yarn run prettier:check && yarn run lint"
+      "pre-push": "yarn run prettier:check && yarn run lint && yarn run validate-queries"


I mean, we don't currently. I'm okay with adding it since I always push with --no-verify anyway.

jimbo · 2020-01-29T20:37:28Z

packages/venia-concept/.graphqlconfig

@@ -8,7 +8,7 @@
                },
                "validate-magento-pwa-queries": {
                    "clients": ["apollo", "literal"],
-                    "filesGlob": "src/**/*.{js,graphql,gql}"
+                    "filesGlob": "../{venia-ui,venia-concept}/{lib,src}/**/**/*.{js,graphql,gql}"


I don't think you do.

Signed-off-by: Stephen Rugh <rugh@adobe.com>

jimbo

Fine by me. Curious to see QA's take.

jimbo · 2020-01-29T21:35:48Z

packages/graphql-cli-validate-magento-pwa-queries/lib/index.js

    }));

-    const ruleDefinition = ['error', ...clientRules];
+    const ruleDefinition = ['warn', ...clientRules];


Yes, in this repo, we do prefer errors over warnings. That said, if you'd like to create an issue to fix all the warnings (and then flip it back from warn to error), that would be fine.

Signed-off-by: Stephen Rugh <rugh@adobe.com>

sirugh · 2020-01-29T22:17:23Z

packages/graphql-cli-validate-magento-pwa-queries/lib/index.js

+                ...clients.map(clientName => ({
+                    env: clientName,
+                    schemaJsonFilepath: schemaPath,
+                    requiredFields: ['id']


The Required Fields rule validates that any specified required field is part of the query, but only if that field is available in schema. This is useful to ensure that query results are cached properly in the client.

https://github.com/apollographql/eslint-plugin-graphql#required-fields-validation-rule

jimbo

@dpatil-magento Theoretically, all of these id additions to the queries should make them hit the cache better. When you test this branch, let me know if you notice any perceptible difference in query performance.

dpatil-magento · 2020-01-30T19:05:33Z

@jimbo Don't see any performance degradation on this PR, so merging this one.

m2-community-project bot added this to Ready for Review in Pull Request Progress Jan 10, 2020

PWAStudioBot added the pkg:venia-ui label Jan 10, 2020

Update gql validator tool to work with venia-ui

36dfea1

Signed-off-by: Stephen Rugh <rugh@adobe.com>

sirugh force-pushed the rugh/fix-gql-validator branch from 969c74c to 36dfea1 Compare January 24, 2020 22:25

validate queries pre push

584aa44

Signed-off-by: Stephen Rugh <rugh@adobe.com>

PWAStudioBot added pkg:graphql-cli-validate-magento-pwa-queries pkg:venia-concept labels Jan 24, 2020

Change rules to warnings

e6b0811

Signed-off-by: Stephen Rugh <rugh@adobe.com>

sirugh added the version: Patch This changeset includes backwards compatible bug fixes. label Jan 24, 2020

sirugh commented Jan 24, 2020

View reviewed changes

m2-community-project bot assigned sirugh Jan 24, 2020

m2-community-project bot moved this from Ready for Review to Review in Progress in Pull Request Progress Jan 24, 2020

m2-community-project bot added the Progress: review label Jan 24, 2020

sirugh commented Jan 24, 2020

View reviewed changes

fix config json

51ae8aa

Signed-off-by: Stephen Rugh <rugh@adobe.com>

sirugh commented Jan 24, 2020

View reviewed changes

sirugh marked this pull request as ready for review January 24, 2020 22:59

sirugh changed the title ~~Initial attempt at fixing validator for venia-ui~~ [chore] Initial attempt at fixing validator for venia-ui Jan 28, 2020

Merge branch 'develop' into rugh/fix-gql-validator

7c93afe

jimbo previously approved these changes Jan 29, 2020

View reviewed changes

m2-community-project bot assigned jimbo Jan 29, 2020

m2-community-project bot moved this from Review in Progress to Reviewer Approved in Pull Request Progress Jan 29, 2020

m2-community-project bot added Progress: approved and removed Progress: review labels Jan 29, 2020

sirugh and others added 2 commits January 29, 2020 15:11

Merge branch 'develop' into rugh/fix-gql-validator

9e27749

Remove from prepush and fix graphql config

fb3eea3

Signed-off-by: Stephen Rugh <rugh@adobe.com>

sirugh dismissed jimbo’s stale review via 7961316 January 29, 2020 21:25

sirugh added 2 commits January 29, 2020 15:26

Remove prepush hook and husky

f1da8cf

Signed-off-by: Stephen Rugh <rugh@adobe.com>

Fix tests

567be69

Signed-off-by: Stephen Rugh <rugh@adobe.com>

sirugh force-pushed the rugh/fix-gql-validator branch from 7961316 to 567be69 Compare January 29, 2020 21:27

jimbo previously approved these changes Jan 29, 2020

View reviewed changes

add id cache requirement. Fix errors

32b35da

Signed-off-by: Stephen Rugh <rugh@adobe.com>

sirugh dismissed jimbo’s stale review via 32b35da January 29, 2020 22:15

sirugh commented Jan 29, 2020

View reviewed changes

jimbo approved these changes Jan 29, 2020

View reviewed changes

Merge branch 'develop' into rugh/fix-gql-validator

6997ae8

dpatil-magento merged commit f9c4b9a into develop Jan 30, 2020

m2-community-project bot moved this from Reviewer Approved to Done in Pull Request Progress Jan 30, 2020

dpatil-magento deleted the rugh/fix-gql-validator branch January 30, 2020 19:06

m2-community-project bot removed the Progress: approved label Jan 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[chore] Initial attempt at fixing validator for venia-ui #2095

[chore] Initial attempt at fixing validator for venia-ui #2095

sirugh commented Jan 10, 2020 •

edited

PWAStudioBot commented Jan 10, 2020 •

edited

sirugh Jan 24, 2020

jimbo Jan 29, 2020

sirugh Jan 29, 2020

sirugh Jan 29, 2020

sirugh Jan 29, 2020

sirugh Jan 24, 2020

sirugh Jan 24, 2020

jimbo Jan 29, 2020

sirugh Jan 24, 2020

jimbo Jan 29, 2020

sirugh commented Jan 24, 2020

jimbo Jan 29, 2020

jimbo Jan 29, 2020

jimbo left a comment

jimbo Jan 29, 2020

sirugh Jan 29, 2020

jimbo left a comment

dpatil-magento commented Jan 30, 2020

[chore] Initial attempt at fixing validator for venia-ui #2095

[chore] Initial attempt at fixing validator for venia-ui #2095

Conversation

sirugh commented Jan 10, 2020 • edited

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Jan 10, 2020 • edited

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

sirugh commented Jan 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimbo left a comment

Choose a reason for hiding this comment

dpatil-magento commented Jan 30, 2020

sirugh commented Jan 10, 2020 •

edited

PWAStudioBot commented Jan 10, 2020 •

edited