-
Notifications
You must be signed in to change notification settings - Fork 373
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
fix(build): fix tree shaking and bundle size #1343
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
bb9ff1e
to
d6f1a0c
Compare
// Based on deep-freeze by substack (public domain) | ||
function deepFreeze(o) { | ||
Object.freeze(o) | ||
Object.getOwnPropertyNames(o).forEach(function(prop) { | ||
if ( | ||
o[prop] !== null && | ||
(typeof o[prop] === "object" || typeof o[prop] === "function") && | ||
!Object.isFrozen(o[prop]) | ||
) { | ||
deepFreeze(o[prop]) | ||
} | ||
}) | ||
return o | ||
} | ||
|
||
// Don't accidentally mutate config as part of the build | ||
deepFreeze(bundles) |
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.
Deleted in favor of Typescript static analysis.
Possibly related #1263 |
d6f1a0c
to
122e0bc
Compare
122e0bc
to
ec0df9d
Compare
ec0df9d
to
b1029ec
Compare
@@ -1,7 +1,7 @@ | |||
import { interpolate, UNICODE_REGEX } from "./context" | |||
import { isString, isFunction } from "./essentials" | |||
import { date, number } from "./formats" | |||
import { compileMessage } from "./compile" | |||
import { compileMessage } from "@lingui/core/compile" |
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.
That's very important change. If we would access this with relative path, tree shaker would not be able to eliminate this import.
Because of package import, tree shaker first looks into package's pakage.json, see {sideEffects: false}
and then remove it from the bundle.
export { | ||
setupI18n, | ||
I18n, | ||
} from "./i18n" | ||
|
||
export type { | ||
AllMessages, | ||
MessageDescriptor, | ||
Messages, |
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.
Without typescript plugin, rollup treat type only exports as regular, and obviously fail. Here we do a little help for him and manually mark them as type only.
@@ -41,7 +41,8 @@ | |||
"loader-utils": "^2.0.0" | |||
}, | |||
"devDependencies": { | |||
"memory-fs": "^0.5.0" | |||
"memory-fs": "^0.5.0", | |||
"webpack": "^4.44.2" |
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.
That was defined on monorepo root but actually used here, i fixed it.
import ora from "ora"; | ||
import chalk from "chalk"; | ||
|
||
const codeFrame = require("@babel/code-frame") |
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 package doesn't have typings
b1029ec
to
03b85f6
Compare
node: 8, | ||
browsers: "> 1%, last 2 versions" | ||
node: 14, | ||
browsers: "> 1%, last 2 versions, not dead" |
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.
not dead
is recommended by browserlist itself. Otherwise, query last 2 versions
will pull internet explorer and other dead browsers.
import babelConfig from './babel.config'; | ||
|
||
import ora from "ora"; | ||
import chalk from "chalk"; |
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 i tried to update chalk
to the latest version, but unfortunately it distributed as ESM package and you can not import ESM package from cjs code. So we stick with this version for a while.
@thekip thank you! 🚀 |
Guys, I came with bad news. During working with
size-limit
i noticed that we don't test for the size real entries written in package.json such asbuild/index.js
build/esm/inde.js
Instead,
size-limit
shamelessly set up to test aproduction
directly. Assuming that user's bundlers will also consume a production bundle somehow.I decided to setup size-limit to the real entry, and it showed completely different results. The bundle size increased almost twice.
Firstly, i thought this is a problem with
size-limit
itself, maybe it doesn't replaceprocess.env.NODE_ENV === "production"
or doesn't apply treeshaking.But after hours of digging in the size-limit sourcecode and webpack / esbuild documentation, i've found that
size-limit
pointing to a real problem with current ESM builds.The problem
Current ESM entries written as:
Assuming the bundler will replace
process.env.NODE_ENV === "production"
to thetrue
and then onlyproduction
branch would be retained after tree-shaking.Unfortunately, bundlers don't work like that. They do replace ENV statement but not tree-shake a "conditional" import. Bundlers are not smart enough and they are very conservative. This code is just not safe to be tree-shaked from theirs point of view.
So our current ESM builds (which are consumed by all bundlers and shipped to the browser) contains both development and production builds together with bundled dependencies (this is another issue)
Proofs:
https://esbuild.github.io/api/#conditionally-injecting-a-file
Try it yourself
Solution
According to the doc to have this imports to be treeshaked each file should be in its own folder with
package.json
with{sideEffect:false}
I didn't go that way, because I think that producing "production" and "development" bundles should be up to the user's bundler.
It had a sense when library produced as UMD module which you can directly use in browser consuming from CDN.
But for NPM library it's useless because it's going to be bundled and minified by the user's bundler.
So I changed the build process to produce only one version of the bundle, with conditions and left the rest of the job for the consumers bundler.
This make a build process so much simpler and so much closer to the modern build practices.
Issue with dependencies
Most of the dependencies were not externalized. Mean they were bundled together with library code.
Especially
babel/core
and@format-message/parser
. That happened becauseexternals
check in rollup options had a logical issue.I fixed it and wrote more robust check. Everethyng from
node_modules
should be externalized + packages from monorepo.What was changed & updated
package.json
files. Addedexports
field where it was not presentedI also think to change babel to esbuild, but this is not decided yet.