-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat: Improve Next.js/CRA/Angular bundle sizes #7842
feat: Improve Next.js/CRA/Angular bundle sizes #7842
Conversation
Codecov Report
@@ Coverage Diff @@
## main #7842 +/- ##
=======================================
Coverage 74.25% 74.25%
=======================================
Files 215 215
Lines 13473 13473
Branches 2646 2646
=======================================
Hits 10004 10004
Misses 3271 3271
Partials 198 198
Continue to review full report at Codecov.
|
@@ -60,6 +60,9 @@ | |||
"lib/index.js": "./enhance-rn.js", | |||
"./src/StorageHelper": "./src/StorageHelper-rn.js" | |||
}, | |||
"browser": { | |||
"crypto": false |
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 would like @manueliglesias to weigh in here. Since we just went through some fixes for Crypto
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.
It's worth another pair of eyes for sure.
For clarity, this line prevents Webpack 4 (& Next.js) from including a crypto
polyfill when it sees require('crypto')
in our code.
This is because only CRA supports https://github.com/aws-amplify/amplify-js/blob/ad2dd864ffbe3bc5c620c3078f82805a4ed83439/packages/amazon-cognito-identity-js/src/utils/cryptoSecureRandomInt.web.js, but this line makes https://github.com/aws-amplify/amplify-js/blob/main/packages/amazon-cognito-identity-js/src/utils/cryptoSecureRandomInt.js not bloat up because of require('crypto')
.
…js into nextjs-bundle-size
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 added comments to the whole PR to navigate 👀 to what matters & why.
@@ -0,0 +1,17 @@ | |||
# This file is used by the build system to adjust CSS and JS output to support the specified browsers below. |
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.
"Eric, why did you commit all of this Angular stuff!? Such bloat! Such wow!"
The bundle size tests should match what our "Getting Started" docs instruct users to do.
So if the default is ng init
, this is the output of ng init
+ a bit Amplify.
This way, when a new version comes out of Angular, we can rm -rf angular
, ng init
the new boilerplate, and unstage the few lines of Amplify.
Hand-picking files/folders to delete would be a PITA.
(window as any).global = window; | ||
(window as any).process = { | ||
env: { DEBUG: undefined }, | ||
}; | ||
|
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.
This comes from our getting started guide ☝️
import { Amplify, Auth } from 'aws-amplify'; | ||
Amplify.configure(); | ||
Auth.currentAuthenticatedUser() | ||
.then(console.info) | ||
.catch(console.warn); | ||
|
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.
This is all the Amplify code we need to test tree-shaking ☝️
}, | ||
"scripts": { | ||
"preanalyze": "$_ build", | ||
"analyze": "source-map-explorer 'build/static/js/*.js'", |
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.
yarn analyze
to see the CRA bundle output in a grid view.
"preanalyze": "$_ build", | ||
"analyze": "source-map-explorer 'build/static/js/*.js'", | ||
"precalculate": "$_ run build", | ||
"calculate": "bundlewatch", |
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.
yarn calculate
is used by the GitHub action to test bundle sizes
}, | ||
{ | ||
"path": "build/static/js/2.*.chunk.js", | ||
"maxSize": "90kB" |
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.
Note that bundlewatch reports GZIP sizes instead of raw
@@ -0,0 +1,23 @@ | |||
# See https://help.github.com/articles/ignoring-files/ for more about ignoring files. |
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.
Same thing as the Angular note above ☝️ rm -rf cra && yarn create react-app
should make this a breeze to test against new versions of CRA.
import { Amplify, Auth } from 'aws-amplify'; | ||
|
||
Amplify.configure(); | ||
|
||
function App() { | ||
useEffect(() => { | ||
Auth.currentAuthenticatedUser() | ||
.then(console.log) | ||
.catch(console.warn); | ||
}, []); |
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.
All we need to test tree-shaking in CRA
@@ -0,0 +1,5 @@ | |||
const withBundleAnalyzer = require('@next/bundle-analyzer')({ | |||
enabled: process.env.ANALYZE === 'true', |
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.
yarn analyze
will output the server + client grid view of bundles
Amplify.configure(); | ||
|
||
export default function Home() { | ||
useEffect(() => { | ||
Auth.currentAuthenticatedUser() | ||
.then(console.log) | ||
.catch(console.warn); | ||
}, []); |
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.
All we need to test tree-shaking in Next.js ☝️
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.
LGTM 🌮
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
Issue #, if available: Fixes #7570
This PR investigates why Next.js bundle size with
Auth
is so large.crypto: false
to preventrequire('crypto')
polyfills in the browserWorkaround – Remove
crypto
polyfill for Next.jsBecause
create-react-app
resolves.web.js
, #7521 improved bundle-size.We can do the same via
next.config.js
:The results are:
Down to 142kB from 281kB, a savings of 138kB.
My sample application (using
Auth
andAPI
) seems to work the same, so it looks like Next.js is polyfillingcrypto
in the browser only:Proposals
Document this workaround with
latest
today?https://docs.amplify.aws/start/getting-started/nextsteps/q/integration/next
Open an issue with Next.js to support
.web.js
Remove
crypto
viabrowser
inamazon-cognito-identity-js
:.web.js
files for Next.js:Research
Next.js baseline
Notice the difference between
/
and/Amplify+Auth
(~215kB):This PR also introduces caps per-page to catch any future regressions:
CRA baseline
Based on the following
App.js
:Why so many
bn.js
?Looks like
crypto
polyfills, which #7521 solved for CRA.Resources
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.