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.

Sign up for GitHub

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

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

Merged
merged 14 commits into from
Jun 8, 2021
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
fa7a5b0
Some preliminary work to get introspection working in v4
tjwiebell May 24, 2021
f8a6f42
Get new cli validation passing following the current eslint rules
tjwiebell May 27, 2021
063b1bc
Minor cleanup
tjwiebell May 27, 2021
f005420
Remove package from workspaces so it doesn't pollute node_modules wit…
tjwiebell May 27, 2021
2b15e9a
Restore additional graphql validation through linting rules
tjwiebell Jun 2, 2021
99068e6
- Re-add package dependencies that were being hidden by hoisting
tjwiebell Jun 2, 2021
74243f0
Test adding additional files to copy command
tjwiebell Jun 2, 2021
4252a9a
Back out change that didn't work
tjwiebell Jun 2, 2021
5f025c7
Merge branch 'develop' into tommy/rename-validator
dpatil-magento Jun 2, 2021
b9de346
Address PR feedback and resolve validation errors with name collisions.
tjwiebell Jun 3, 2021
0f56922
Fix missed @client directive
tjwiebell Jun 3, 2021
52ed88d
Remove codegen section until we can resolve name conflicts
tjwiebell Jun 3, 2021
f94d9dc
Colocate client schemas with components
tjwiebell Jun 4, 2021
5b7fd9c
Merge branch 'develop' into tommy/rename-validator
dpatil-magento Jun 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
"packages/babel-preset-peregrine",
"packages/create-pwa",
"packages/extensions/*",
"packages/graphql-cli-validate-magento-pwa-queries",
"packages/pagebuilder",
"packages/peregrine",
"packages/pwa-buildpack",
Expand Down
8 changes: 4 additions & 4 deletions packages/peregrine/lib/talons/CartPage/GiftOptions/useGiftOptions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ const useGiftOptions = props => {
id: cartId,
include_gift_receipt: includeGiftReceipt,
include_printed_card: includePrintedCard,
gift_message: giftMessage,
local_gift_message: giftMessage,
...optionsToUpdate
}
}
Expand Down Expand Up @@ -85,7 +85,7 @@ const useGiftOptions = props => {
return throttle(
(updateGiftOptions, newGiftMessage) => {
updateGiftOptions({
gift_message: newGiftMessage
local_gift_message: newGiftMessage
});
},
1000,
Expand Down Expand Up @@ -127,12 +127,12 @@ const useGiftOptions = props => {
const {
include_gift_receipt,
include_printed_card,
gift_message
local_gift_message
} = data.cart;

setIncludeGiftReceipt(include_gift_receipt);
setIncludePrintedCard(include_printed_card);
setGiftMessage(gift_message);
setGiftMessage(local_gift_message);
}
}, [setIncludeGiftReceipt, setIncludePrintedCard, data]);

Expand Down
4 changes: 2 additions & 2 deletions packages/peregrine/lib/talons/CheckoutPage/BillingAddress/billingAddress.gql.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import { AvailablePaymentMethodsFragment } from '../PaymentInformation/paymentIn
/* eslint-disable graphql/template-strings */
export const GET_IS_BILLING_ADDRESS_SAME = gql`
query getIsBillingAddressSame($cartId: String!) {
cart(cart_id: $cartId) @client {
cart(cart_id: $cartId) {
id
isBillingAddressSame
isBillingAddressSame @client
}
}
`;
Expand Down
8 changes: 4 additions & 4 deletions packages/peregrine/lib/talons/CheckoutPage/PaymentInformation/creditCard.gql.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,18 @@ import { AvailablePaymentMethodsFragment } from './paymentInformation.gql';
/* eslint-disable graphql/template-strings */
export const GET_IS_BILLING_ADDRESS_SAME = gql`
query getIsBillingAddressSame($cartId: String!) {
cart(cart_id: $cartId) @client {
cart(cart_id: $cartId) {
id
isBillingAddressSame
isBillingAddressSame @client
}
}
`;

export const GET_PAYMENT_NONCE = gql`
query getPaymentNonce($cartId: String!) {
cart(cart_id: $cartId) @client {
cart(cart_id: $cartId) {
id
paymentNonce
paymentNonce @client
}
}
`;
Expand Down
2 changes: 1 addition & 1 deletion packages/venia-concept/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ dist
.env
build-stats.json
npm-debug.log
lastCachedGraphQLSchema.json
merged-schema*.json
test-report.xml
test-results.json
yarn-error.log
18 changes: 2 additions & 16 deletions packages/venia-concept/.graphqlconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,11 @@
{
"projects": {
"venia": {
"schemaPath": "lastCachedGraphQLSchema.json",
"schemaPath": "merged-schema.v3.json",
Copy link
Contributor

Choose a reason for hiding this comment

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

v3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Config structure for graphql-config@3

"includes": ["**/client-schema.graphql"],
"extensions": {
"endpoints": {
"default": "${env:MAGENTO_BACKEND_URL}/graphql"
},
"validate-magento-pwa-queries": {
"clients": [
"apollo",
"literal"
],
"magentoBackendEdition": "${env:MAGENTO_BACKEND_EDITION}",
"filesGlob": "../{peregrine,venia-ui,venia-concept}/{lib,src}/**/*.{js,graphql,gql}",
"ceFilesGlob": "../{peregrine,venia-ui,venia-concept}/{lib,src}/**/*.ce.js",
"eeFilesGlob": "../{peregrine,venia-ui,venia-concept}/{lib,src}/**/*.ee.js",
"ignore": [
"../venia-ui/lib/**/giftOptions.js",
"../venia-ui/lib/**/wishlistPage.gql.js",
"../venia-ui/lib/**/wishlistItem.gql.js"
]
}
}
}
Expand Down
14 changes: 14 additions & 0 deletions packages/venia-concept/.graphqlrc.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
projects:
default:
schema:
- ${MAGENTO_BACKEND_URL}/graphql
- "**/client-schema.graphql"
documents:
- '../{peregrine,venia-ui,venia-concept}/{lib,src}/**/!(*.spec).js'
- '!**/*.ce.js'
extensions:
codegen:
generates:
./merged-schema.v4.json:
plugins:
- introspection
15 changes: 15 additions & 0 deletions packages/venia-concept/client-schema.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
extend type Query {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

customerWishlistProducts: [String!]!
}

extend type Cart {
include_gift_receipt: Boolean!
include_printed_card: Boolean!
isBillingAddressSame: Boolean!
local_gift_message: String
paymentNonce: String
}

extend type Currency {
current_currency_code: String!
}
11 changes: 7 additions & 4 deletions packages/venia-concept/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,15 @@
"build:report": "buildpack generate-build-report",
"buildpack": "buildpack",
"clean": "rimraf dist",
"download-schema": "graphql get-schema --project venia --insecure",
"download-schema": "graphql-codegen --require dotenv/config",
"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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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/

Copy link
Contributor Author

@tjwiebell tjwiebell Jun 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed from build:dev in b9de346

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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):

  1. force a graphql resolution
  2. 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}",
  3. overcome default node_module exclusion by adding ignorePattern: '!node_modules/*' to linterConfiguration
    to node_modules/graphql-cli-validate-magento-pwa-queries/lib/index.js
  4. Profit? [bug]: Query uses non existing WishlistVisibilityEnum in CE #3217

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

"validate-queries": "graphql validate --apollo --keepClientFields --noStrictFragments --require dotenv/config",
"watch": "webpack-dev-server --progress --color --env.mode development"
},
"repository": "github:magento/pwa-studio",
Expand All @@ -51,6 +51,9 @@
"@babel/plugin-transform-runtime": "~7.4.4",
"@babel/preset-env": "~7.3.4",
"@babel/runtime": "~7.4.2",
"@graphql-cli/codegen": "~1.17.23",
"@graphql-cli/validate": "~2.1.0",
"@graphql-codegen/introspection": "~1.18.2",
"@magento/babel-preset-peregrine": "~1.1.0",
"@magento/eslint-config": "~1.5.0",
"@magento/pagebuilder": "~5.0.0",
Expand Down Expand Up @@ -85,8 +88,8 @@
"express": "~4.16.4",
"file-loader": "~4.0.0",
"graphql": "~15.3.0",
"graphql-cli": "~3.0.11",
"graphql-cli-validate-magento-pwa-queries": "~1.4.0",
"graphql-cli": "~4.1.0",
"graphql-config": "~3.3.0",
"html-webpack-plugin": "~3.2.0",
"informed": "~3.27.0",
"jarallax": "~1.11.1",
Expand Down
8 changes: 4 additions & 4 deletions packages/venia-ui/lib/components/CartPage/PriceAdjustments/GiftOptions/giftOptions.gql.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@ import { gql } from '@apollo/client';
*/
const GET_GIFT_OPTIONS = gql`
query getGiftOptions($cartId: String!) {
cart(cart_id: $cartId) @client {
cart(cart_id: $cartId) {
id
include_gift_receipt
include_printed_card
gift_message
include_gift_receipt @client
include_printed_card @client
local_gift_message @client
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the rename here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
}
`;
Expand Down
5 changes: 3 additions & 2 deletions packages/venia-ui/lib/components/CheckoutPage/PaymentInformation/paymentInformation.gql.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,16 @@ export const GET_PAYMENT_INFORMATION = gql`
}
${AvailablePaymentMethodsFragment}
`;

// We disable linting for local fields because there is no way to add them to
// the fetched schema.
// https://github.com/apollographql/eslint-plugin-graphql/issues/99
/* eslint-disable graphql/template-strings */
export const GET_PAYMENT_NONCE = gql`
query getPaymentNonce($cartId: String!) {
cart(cart_id: $cartId) @client {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted these directive changes in b9de346

cart(cart_id: $cartId) {
id
paymentNonce
paymentNonce @client
}
}
`;
Expand Down
Loading