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

Remove react-query-devtools from production build #1121

Merged
merged 14 commits into from
Apr 12, 2023

Conversation

kevinxh
Copy link
Collaborator

@kevinxh kevinxh commented Apr 10, 2023

Description

Goal: remove react-query-devtools from production build to reduce bundle size.

Changes:

  • removes react-query-devtools from commerce-sdk-react
  • install react-query-devtools in retail react app
  • Lower bundle size test thresholds.

Bundle analysis before & after

BEFORE

Screenshot 2023-04-10 at 11 41 02 AM

AFTER

Screenshot 2023-04-10 at 11 38 16 AM

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

How to Test-Drive This PR

  • run npm start in ./packages/template-retail-react-app and open localhost:3000 and notice the devtool icon on left bottom corner
  • run NODE_ENV=production npm start, verify the icon is gone
  • run npm run analyze-build and verify the react-query-devtools is not in the build

@kevinxh kevinxh changed the title remove react-query-devtools from hook library Remove react-query-devtools from production build Apr 10, 2023
@kevinxh kevinxh marked this pull request as ready for review April 10, 2023 18:54
@kevinxh kevinxh requested a review from a team as a code owner April 10, 2023 18:54
alexvuong
alexvuong previously approved these changes Apr 10, 2023
wjhsf
wjhsf previously approved these changes Apr 10, 2023
},
{
"path": "build/vendor.js",
"maxSize": "355 kB"
"maxSize": "285 kB"
Copy link
Contributor

Choose a reason for hiding this comment

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

📉

@@ -57,6 +58,7 @@ const AppConfig = ({children, locals = {}}) => {
<MultiSiteProvider site={locals.site} locale={locals.locale} buildUrl={locals.buildUrl}>
<ChakraProvider theme={theme}>{children}</ChakraProvider>
</MultiSiteProvider>
<ReactQueryDevtools />
Copy link
Contributor

Choose a reason for hiding this comment

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

From the docs:

By default, React Query Devtools are only included in bundles when process.env.NODE_ENV === 'development', so you don't need to worry about excluding them during a production build.

👌

@kevinxh
Copy link
Collaborator Author

kevinxh commented Apr 10, 2023

hmmmm, why is bundlesize test give different results on CI...

Screenshot 2023-04-10 at 12 03 38 PM

(CI)

Screenshot 2023-04-10 at 12 04 56 PM

(local)

@vmarta
Copy link
Contributor

vmarta commented Apr 10, 2023

hmmmm, why is bundlesize test give different results on CI...

That's strange 🤔 Is the generated project really different than what's in the template-retail-react-app folder?

@alexvuong
Copy link
Collaborator

alexvuong commented Apr 10, 2023

I think the difference is that in generated-project, the pwa-kit-react-sdk/pwa-kit-*/commerce-sdk-react is included inside vendor.js while in the template, it is included inside main.js. If you run analyze-build on generated-project, and the template, you will see it. I ran both scenarios on v2.7.0, and v2, and I found pwa-kit-react-sdk caused the difference in bundle sizes
V2
image
generated project
image

@kevinxh kevinxh dismissed stale reviews from wjhsf and alexvuong via 5b251ac April 10, 2023 21:59
@kevinxh
Copy link
Collaborator Author

kevinxh commented Apr 10, 2023

Thanks @alexvuong . I made a change to CI to not run bundlesize in generated_projects.

@alexvuong
Copy link
Collaborator

@kevinxh I am wondering why there is a difference between generated project and the template like that. Is it because of template is inside a monorepo? 🤔 .

@kevinxh
Copy link
Collaborator Author

kevinxh commented Apr 10, 2023

@alexvuong yea, i think so. I don't think it's super important to choose to run bundle size in either monorepo or generated project. It's a reference for us to not go over the limit, the absolute value isn't super useful. The reason why I chose to run bundlesize in the monorepo is to get the same result when developing locally when you run npm run test:max-file-size.

@kevinxh kevinxh requested review from alexvuong and wjhsf April 10, 2023 22:39
@bendvc
Copy link
Collaborator

bendvc commented Apr 11, 2023

@alexvuong yea, i think so. I don't think it's super important to choose to run bundle size in either monorepo or generated project. It's a reference for us to not go over the limit, the absolute value isn't super useful. The reason why I chose to run bundlesize in the monorepo is to get the same result when developing locally when you run npm run test:max-file-size.

@kevinxh I think it's important that both analyze-build return the same results in the mono repo and the generated project. This would overall jsut make the landscape less confusing, and I feel that the generated project IS the standard where we ultimately want to check bundle size.

You might want to play around with the following part of the webpack configuration:

                    vendor: {
                        // Anything imported from node_modules lands in
                        // vendor.js, if we're chunking.
                        test: /node_modules/,
                        name: 'vendor',
                        chunks: 'all'
                    }

You'll see that this rule isn't catching the imports when run in the mono-repo, maybe there is a quick fit to check for /pwa-kit/ folder so those can go into the vendor as well.

@kevinxh
Copy link
Collaborator Author

kevinxh commented Apr 12, 2023

@alexvuong yea, i think so. I don't think it's super important to choose to run bundle size in either monorepo or generated project. It's a reference for us to not go over the limit, the absolute value isn't super useful. The reason why I chose to run bundlesize in the monorepo is to get the same result when developing locally when you run npm run test:max-file-size.

@kevinxh I think it's important that both analyze-build return the same results in the mono repo and the generated project. This would overall jsut make the landscape less confusing, and I feel that the generated project IS the standard where we ultimately want to check bundle size.

You might want to play around with the following part of the webpack configuration:

                    vendor: {
                        // Anything imported from node_modules lands in
                        // vendor.js, if we're chunking.
                        test: /node_modules/,
                        name: 'vendor',
                        chunks: 'all'
                    }

You'll see that this rule isn't catching the imports when run in the mono-repo, maybe there is a quick fit to check for /pwa-kit/ folder so those can go into the vendor as well.

Updated the vendor.js chunking rule! Thanks for the suggestion!

bendvc
bendvc previously approved these changes Apr 12, 2023
@kevinxh kevinxh requested a review from alexvuong April 12, 2023 17:39
@kevinxh kevinxh enabled auto-merge (squash) April 12, 2023 17:40
alexvuong
alexvuong previously approved these changes Apr 12, 2023
@kevinxh kevinxh dismissed stale reviews from alexvuong and bendvc via 2116e67 April 12, 2023 17:55
@kevinxh kevinxh merged commit a4700b2 into v3 Apr 12, 2023
@kevinxh kevinxh deleted the remove-react-query-devtools branch April 12, 2023 18:52
bfeister added a commit that referenced this pull request Apr 25, 2023
* v3:
  [Spike]Replace watch with nodemon (#1146)
  Fix Page Designer ImageWithText Link component (#1092) (#1148)
  Upgrade deprecated dependencies (#1124)
  Restart login flow if refresh_token is invalid (#1135)
  Move some util to site-utils to avoid circular imports (#1133)
  Restore old file name. (#1140)
  Update eslint configuration (#1129)
  CI: clarify the environment variables (#1127)
  Remove react-query-devtools from production build (#1121)
  Update lerna.json (#1118)
  Parallelize lighthouse ci (#1126)
  Increase test timeouts only on CI env (#1123)
  Remove unused `request` deprecated dependency. (#1125)
  Upgrade msw to latest (#1100)
  Add mergeBasket hook (#1114)
  [V3][Hooks Integration 🪝] Manually update cache for ShopperCustomer (#1113)
  dont use callback on mutateAsync (#1119)
  2-spaces not 4-spaces (#1117)

# Conflicts:
#	packages/internal-lib-build/package-lock.json
#	packages/pwa-kit-create-app/package-lock.json
#	packages/pwa-kit-dev/package-lock.json
#	packages/pwa-kit-dev/package.json
#	packages/pwa-kit-dev/src/configs/webpack/config.js
#	packages/pwa-kit-dev/src/ssr/server/build-dev-server.js
#	packages/pwa-kit-react-sdk/package-lock.json
#	packages/pwa-kit-runtime/package-lock.json
#	packages/template-express-minimal/package-lock.json
#	packages/template-mrt-reference-app/package-lock.json
#	packages/template-retail-react-app/package-lock.json
#	packages/template-typescript-minimal/package-lock.json
#	packages/test-commerce-sdk-react/package-lock.json
bfeister added a commit that referenced this pull request Apr 25, 2023
…-rehaul

* feature/template-extensibility:
  [Spike]Replace watch with nodemon (#1146)
  Fix Page Designer ImageWithText Link component (#1092) (#1148)
  Upgrade deprecated dependencies (#1124)
  Restart login flow if refresh_token is invalid (#1135)
  Move some util to site-utils to avoid circular imports (#1133)
  Restore old file name. (#1140)
  Update eslint configuration (#1129)
  CI: clarify the environment variables (#1127)
  Remove react-query-devtools from production build (#1121)
  Update lerna.json (#1118)
  Parallelize lighthouse ci (#1126)
  Increase test timeouts only on CI env (#1123)
  Remove unused `request` deprecated dependency. (#1125)
  Upgrade msw to latest (#1100)
  Add mergeBasket hook (#1114)
  [V3][Hooks Integration 🪝] Manually update cache for ShopperCustomer (#1113)
  dont use callback on mutateAsync (#1119)
  2-spaces not 4-spaces (#1117)

# Conflicts:
#	packages/internal-lib-build/package-lock.json
#	packages/pwa-kit-dev/package-lock.json
#	packages/pwa-kit-react-sdk/package-lock.json
#	packages/pwa-kit-runtime/package-lock.json
#	packages/pwa-kit-runtime/package.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants