feat: quality of life improvements for graphql file generation#982
feat: quality of life improvements for graphql file generation#982matthewvolk merged 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
5 Ignored Deployments
|
🦋 Changeset detectedLatest commit: f50b525 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
matthewvolk
force-pushed
the
feat/provider-naming-convention
branch
from
bb6a9d4 to
9abefa5
Compare
There was a problem hiding this comment.
Why are we removing this? Shouldn't this point to bigcommerce-graphql.d.ts now?
There was a problem hiding this comment.
While in Multi-schema mode, since generateOutput produces an introspection file for each GraphQL schema in the tsconfig plugin schemas array, we can't pass it a single file name - instead, generateOutput will create a file for each tadaOutputLocation key in each schema config object 🙂
If we try to manually set this to:
await generateOutput({
disablePreprocessing: false,
output: join(__dirname, '../bigcommerce-graphql.d.ts'),
tsconfig: undefined,
});We get the following:
There was a problem hiding this comment.
Can you explain why this is needed?
There was a problem hiding this comment.
TL;DR:
- The errors thrown in
getStoreHashandgetTokenare synchronous - If these functions were called synchronously and an error was thrown, the script would immediately terminate (unless they were caught)
- However, these functions are called within an asynchronous context (
generate) - Since we are no longer synchronous, and are not catching errors thrown from calling
generate, thethrowstatements emit anunhandledRejectionevent instead of immediately terminating the program
You can try this by reverting the code so that it looks like:
// ...
generate();Then, remove a required environment variable from .env.local and try pnpm generate again. You'll notice the process exits with a 0 exit code, but no Schema or introspection files were generated.
If you then add:
// ...
generate();
process.on('unhandledRejection', (error) => {
console.error(error);
process.exit(1);
});And run pnpm generate again, you'll see the error logged to the console.
There was a problem hiding this comment.
Update: Refactored this to simply wrap the body of generate in a try/catch, lends to better readability compared to the IIFE
matthewvolk
force-pushed
the
feat/provider-naming-convention
branch
from
9abefa5 to
f50b525
Compare
⚡️🏠 Lighthouse reportLighthouse ran against https://catalyst-latest-adpbl47hi-bigcommerce-platform.vercel.app 🖥️ DesktopWe ran Lighthouse against the changes on a desktop and produced this report. Here's the summary:
📱 MobileWe ran Lighthouse against the changes on a mobile and produced this report. Here's the summary:
|
2 participants
What/Why?
This PR introduces changes that address the following concerns:
A common pattern I'm seeing when integrating Catalyst with various third party services is that some third party services may have their own GraphQL API's that Catalyst needs to interface with; in these cases, I've been leveraging the Multi-schema mode feature of
gql.tada.The moment that a second schema is added to a Catalyst project, the
schema.graphqlandgraphql-env.d.tsfile naming conventions become less useful, as the file names do not easily describe the provider from which the files were created. Changing the naming convention of the default BigCommerce GraphQL files to includebigcommercein the name makes it much easier to identify which GraphQL API the files are generated from, especially when you have multiple GraphQL schema files in a single project.Additionally, I want to propose a change to our opinions of which files should live in version control for both the Catalyst monorepository, and separately, scaffolded Catalyst projects. In the context of working within the monorepository, it makes sense to
.gitignorethese files; we don't want any single test store's schema to be tracked in/core, as this would cause those files to be copied into scaffolded Catalyst projects. However, this opinion changes in the context of scaffolded projects. Scaffolded projects should have their GraphQL schema/introspection files tracked so that developers who are all collaborating on a single storefront build can track changes introduced to GraphQL schemas in pull requests.Finally, this PR also slightly modifies
core/scripts/generate.cjsto prevent errors from being swallowed by the script instead of being logged to the console. Sincegenerateis asynchronous, Node throws anunhandledRejectionevent instead of actually throwing the error in the console. The change introduced catches any errors thrown, and manually logs them to the console.Tip
If this PR is merged, be sure to run
pnpm generatelocally to create required GraphQL files, and then delete the now-untrackedschema.graphqlandgraphql-env.d.tsfiles to help prevent accidentally committing them.Testing
pnpm generate.env.local(such as store hash) and runpnpm generateagain, you'll see the more helpful error messages logged bygenerate.cjs.