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.

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

Bugfix/heuristic fragment matcher #1707

Conversation

Jordaneisenburger
Copy link
Member

@Jordaneisenburger Jordaneisenburger commented Sep 19, 2019

Description

This PR adds the IntrospectionFragmentMatcher to resolve the heuristic fragment matcher warning.

It:

  • Issues an introspection query to the GraphQL schema at build time to get the union and interface types
  • It passes those to the bundle via webpack's DefinePlugin
  • The app uses them to initialize the IntrospectionFragmentMatcher at run time

As such, there is also some refactoring of buildpack to make issuing GraphQL requests easier 🎉 .

Related Issue

Related issues #1474 & #1662

Closes #1474

Verification Steps

  1. Go to /augusta-earrings.html
  2. Open chrome console
  3. See console warning
  4. update you code base with this PR and run yarn run build in project root
  5. Visit /augusta-earrings.html again and, the warning should be gone.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Sep 19, 2019

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

Generated by 🚫 dangerJS against e67e188

@jimbo
Copy link
Contributor

jimbo commented Sep 19, 2019

@Jordaneisenburger Wow, this is so valuable! Thanks for tackling this issue; it's been bothering us lately.

Let's see if we can't find a way to get the backend URL from .env.

Copy link
Contributor

@supernova-at supernova-at left a comment

Choose a reason for hiding this comment

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

I don't know exactly what it looks like yet but I'd really like to move this build-time logic to buildpack.

Thanks for tackling this!

packages/venia-concept/generateGraphQlFragmentTypes.js Outdated Show resolved Hide resolved
packages/venia-concept/generateGraphQlFragmentTypes.js Outdated Show resolved Hide resolved
@supernova-at
Copy link
Contributor

I moved this build-time logic to buildpack, this is ready for review.

@@ -0,0 +1,63 @@
const fetch = require('node-fetch');
Copy link
Contributor

Choose a reason for hiding this comment

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

So GitHub didn't pick it up, but I renamed fetcherUtils to graphQL (and their associated test files).

This is instrumentation to make GraphQL calls so I think this name is more indicative of what this module's job is.

.then(json => json.data)
.catch(err => {
console.error(err);
throw err;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that instead of returning an empty string here we are now re-throwing the caught error.

This allows the caller to determine how they want to handle the error case in a more idiomatic way try / catch instead of having to check success cases against an empty string.

@@ -0,0 +1,12 @@
# Introspect the GraphQL schema to obtain information about its types.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that buildpack now has its own queries folder with .graphql files in it.

This allows us to not have to inline large GraphQL strings 🎉

module.exports = {
getMediaUrl,
getSchemaTypes
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This file isn't strictly necessary, and we'll have to add all new .graphql files here to make them available for the rest of buildpack, but we do need to require the .graphql files in a different way (using requireGraphQL) so this consolidates that logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer that. Having a simple index.js makes things simpler for the future.

packages/pwa-buildpack/package.json Outdated Show resolved Hide resolved
sirugh
sirugh previously approved these changes Oct 8, 2019
Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

Great!

transformIgnorePatterns: ['node_modules/(?!@magento/)'],
globals: {
TEST_GLOBAL: 'true',
UNION_AND_INTERFACE_TYPES: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR introduced a dependency on globals in a way that uncovered a bug in our config. The previous location of the globals configuration did not actually set globals and the test for STORE_NAME hardcoded it in the test spec rather than relying on the config (probably because it was broken).

This now properly creates serializable globals for Jest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]: WARNING: heuristic fragment matching going on!
8 participants