-
Notifications
You must be signed in to change notification settings - Fork 63
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
chore: remove graphql-utils and allow for the use of swc #2176
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f447888:
|
Still haven't started going through the code yet, but since you mentioned that this is a breaking change: should we update the major? How should we communicate to our customers that this change has happened without affecting them? Should we create a small migration guide? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see nothing wrong with this PR other than the point about trying to minimize the impact of people already using API Extension.
Can we add our gql
sigil that calls the function and gives a deprecation notice?
Although it would be the right thing to do according to semver, since we're still in EAP, I don't think we should update the major. Yes, we should communicate. Yes, we should create a migration guide. I'll do it!
Did not get what you meant by this. Can you explain it further? |
Sorry, sigil is the wrong name for what I was trying to say. I meant tag function. Currently our customers use the // inside graphql-utils
import { gql as new_gql } from '@generated'
function gql(string) {
console.warn("Using the gql tag function will be deprecated in version 2.2 please use the gql function exported from the @generated folder")
return new_gq(string)
} |
Got it! We can, but we wouldn't be able to defer to the |
69ca59d
to
63c9c5d
Compare
3ae4188
to
316abc8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are lots of things to go more profoundly here. Fantastic job!
@@ -0,0 +1,111 @@ | |||
# Migrating to 2.2.65 - graphql-utils depreciation | |||
|
|||
From 2.2.60 on, the `@faststore/graphql-utils` is officially deprecated. It was previously used internally by `@faststore/core` to handle GraphQL optimizations and GraphQL definitions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎗️ remember to update it to the correct version.
|
||
From 2.2.60 on, the `@faststore/graphql-utils` is officially deprecated. It was previously used internally by `@faststore/core` to handle GraphQL optimizations and GraphQL definitions. | ||
|
||
The `@faststore/graphql-utils` has been replaced by open-source solutions maintained by the community that are now re-exported from `@faststore/core`. There are minor breaking changes on how developers should write GraphQL definitions and invoke queries and mutations, which were introduced in version 2.2.60. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎗️ also here regarding the version
@@ -11,6 +11,7 @@ | |||
"scripts": { | |||
"generate:schema": "tsx src/server/generator/generateGraphQLSchemaFile.ts", | |||
"generate:codegen": "graphql-codegen", | |||
"generate:copy-back": "copyfiles \"@generated/**/*\" $DESTINATION", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why do we need to copy back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To have proper typings during development. The merger happens on the .faststore
folder, but the store imports types and the gql
helper from @faststore/core
. We copy back from .faststore
to @faststore/core
so the types from the merger (core + store) are served from @faststore/core
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! 🚀
Left some suggestions on the guide and I notice that a release note would be also good to have an official note for the migration, help spread the word to CX, and give more visibility for the guide. I've left this comment with the doc where I'm drafting the release note, and after the team can review it.
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
apps/site/pages/docs/migration-guides/Migrating to 2.2.65 - graphql-utils depreciation.mdx
Outdated
Show resolved
Hide resolved
… gql and query types
Co-authored-by: Mariana Caetano Pereira <[email protected]>
Co-authored-by: Mariana Caetano Pereira <[email protected]> Co-authored-by: Fanny Chien <[email protected]>
b9df284
to
f447888
Compare
## What's the purpose of this pull request? This PR holds the PRs that will be available in 3.x version: - #2184 - #2198 - #2176 Preview PR - vtex-sites/starter.store#360 --------- Co-authored-by: Fanny <[email protected]> Co-authored-by: Ícaro Azevedo <[email protected]> Co-authored-by: Mariana Caetano Pereira <[email protected]>
…/cli (#2377) ## What's the purpose of this pull request? This PR aims to make `@faststore/core` a dependency of the `@faststore/cli`. It just makes sense. Someone can fork `@faststore/core` and never need to use the CLI, but there's hardly any usefulness to the CLI without `@faststore/core`. ## How it works? The only thing linking Core to the CLI was the `generate-graphql` command on the CLI. It just called back 4 scripts on core's `package.json`: `generate:schema`, `generate:codegen`, `format:generated` and `generate:copy-back`. The copy-back command was only needed to improve the DX of customers customizing their GraphQL schema [[ref]](#2176 (comment)) and it was the only part of the script that needed information available at the CLI level only. Since it was only a DX improvement, all that was needed was, during the `faststore dev` execution I had to generate the new schema, copy the files to the correct `node_modules` dir, and then start the NextJS dev server. In order to do this I split the `dev` script into `predev`, `dev`, and `dev-only`*. From the FastStore CLI I could now call `yarn predev`, copy the files to the correct place, and then run `yarn dev-only` to start the NextJS dev server. Someone developing on the faststore monorepo (us VTEX developers, or someone contributing to our project) will continue to run core via `yarn dev`. [npm](https://docs.npmjs.com/cli/v10/using-npm/scripts#pre--post-scripts), [yarn](https://classic.yarnpkg.com/en/docs/cli/run#toc-yarn-run-script), and [pnpm](https://pnpm.io/cli/run#npmrc-settings) support the lifecycle scripts out of the box. I did the same thing to `build` for equivalence. One thing that can be up to discussion is the existence of `runCommandSync` util. It was only used on `generate-graphql` and can be easily removed (in this PR or in a future one). ## How to test it? Add this PR `@faststore/cli` from codesandbox as a dependency of the starter store, remove `@faststore/core` as an explicit dependency, and `@faststore/cli` as a devDependency. Run your store and see that everything still works fine. Or go to the [starter preview](https://sfj-20ca89a--starter.preview.vtex.app/) [[PR]](vtex-sites/starter.store#489) here and see the store running in "production".
What's the purpose of this pull request?
This PR makes
@faststore/core
compatible with SWC by dropping the@faststore/graphql-utils
package, meaning we don't depend on a babel-only plugin built by us.How it works?
Why we did it
SWC is a compiler and bundler used by Next.js. It is the default bundler since Next.js 12 the default code minifier since Next.js 13. It promises considerable performance improvements when compared to babel, its main competitor.
Our use of a custom babel plugin - made available through
@faststore/graphql-utils
- made us unable to use SWC, since babel plugins are not compatible with SWC. This plugin helped us optimize GraphQL queries, improve security of queries and mutations and handle GraphQL operations definitions.When this plugin was first introduced, there wasn't a solution that could useful to us at the community, so we had to develop it ourselves. Now, The Guild's client-preset does exactly what we need. It provides a all-in-one tool for handling persisted queries (optimization & security) and a GraphQL helper (
gql
) for defining operations, among other things we already used as standalone solutions, like type generation for the schema.By using
client-preset
we're no longer bound to babel or SWC exclusively, as their solution doesn't rely on any of them. There are recommended plugins for both platforms, as it helps reduce bundle sizes. We also helps reduce the code we have to maintain, adopt community standards and get up do date in terms of bundling and compilation tools.Results
After the store was migrated to use
client-preset
, I ran a few builds to see if there would be any difference in bundle size. The results were not great, as the SWC version increased the bundle size considerably while reducing the build time in a negligible way (~5s reduction, although ~20% reduction percentage wise).I wasn't able to make the SWC plugin work, and I think that's because we're on Next 12. The support for SWC plugins within Next.js is still experimental (even in Next.js 13) and these errors are expected. It will probably work when we migrate to it, tho.
I also ran a comparison considering the client-preset with the community babel plugin, and the results were comparable to the ones we had before, time-wise as well.
Click on the images to read the data.
Implementation details
operationName
We previously used the operation name to identify queries and mutations on the backend.
client-preset
creates hashes based on the operation name to do the same thing. I updated the code to comply with this change, but added the operation name to the query arguments soenvelop
on theexecute
functionTo do that, I used
client-preset
onExecutableDocumentNode
hook and extracted it manually at thecodegen.ts
config file.@generated dir
Previously, the schema and other GraphQL files were inside the
@generated/graphql
folder. I changed it to@generated
directly so@generated/graphql
imports wouldn't break. It would be a pain to change all files, and we don't currently have other generated files other than graphql files, so it made sense to change it.SWC minification
I enabled SWC minification at
next.config.js
since it is already the default minification tool Next 13. It provided awesome results, decreasing the build time in 10s to 15s.How to test it?
This is a breaking change, as we know use the
gql
helper function and how to use it. We already exported it from@faststore/core/api
, but it was possible to import it from@faststore/graphql-utils
as well. Importing it from@faststore/core/api
is the only possible and recommended way from now on.At the code, users also have to change the
gql
call to an actual function call:This change only affects stores containing API Extensions.
Users shouldn't feel anything different when browsing the website.
Starters Deploy Preview
vtex-sites/starter.store#334
References
https://the-guild.dev/graphql/codegen/plugins/presets/preset-client
https://the-guild.dev/blog/optimize-bundle-size-with-swc-and-graphql-codegen
I read this code a bunch to figure out how client-preset worked and how to use the options: https://github.com/dotansimha/graphql-code-generator/blob/master/packages/presets/client/src/index.ts
https://nextjs.org/docs/messages/swc-minify-enabled
https://nextjs.org/docs/architecture/nextjs-compiler
dotansimha/graphql-code-generator#9057