feat: use dev/prod conditions instead of webpack in exports #5858
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
etrepum
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz and
ivailop7
as code owners
|
As far as I can tell the failure is a flaky collab test. It didn't repro locally on my mac |
etrepum
force-pushed
the
forkless-exports
branch
from
43632bd
to
ba4b668
Compare
etrepum
force-pushed
the
forkless-exports
branch
from
ba4b668
to
c7d0aad
Compare
etrepum
force-pushed
the
forkless-exports
branch
from
c7d0aad
to
59757ca
Compare
| const path = require('node:path'); | ||
| const fs = require('fs-extra'); | ||
|
|
||
| const alias = Object.fromEntries( |
There was a problem hiding this comment.
Pls add comment that explains purpose of this file.
Also can you pls elaborate on why we replace simple config from package.json with this fairly complex file?
There was a problem hiding this comment.
It never really worked correctly, outside of the main lexical package anyway, so that's one reason! :)
When checking the size of @lexical/rich-text or @lexical/plain-text it would have to pull in whatever lexical package was in node_modules (plus their other monorepo dependencies like @lexical/clipboard), which would be some previously published version.
The other reason is that it simply didn't work with the simpler configuration. I don't know why or when it stopped working, but that was the primary motivation. I only discovered it was often wrong even in the best of times when I investigated further.
| }; | ||
| } | ||
|
|
||
| module.exports = ['lexical', '@lexical/rich-text', '@lexical/plain-text'].map( |
There was a problem hiding this comment.
Why only these 3 packages? Can we discover them based on the config property in their dirs/package.json? We already do glob here
There was a problem hiding this comment.
I think that would be an @fantactuka question (size-limit was introduced in #3600), these were the only modules measured in the previous configuration. Now that we do have more infrastructure to support this size-limit tool we have more options but the measurements are only useful if people care about them and I don't know what the team cares about especially because there is no configured limits such that the build would fail.
| @@ -98,7 +98,7 @@ | |||
| "update-version": "node ./scripts/updateVersion", | |||
| "postversion": "git checkout -b ${npm_package_version}__release && npm install && npm run update-version && npm run update-changelog && git add -A && git commit -m v${npm_package_version} && git tag -a v${npm_package_version} -m v${npm_package_version}", | |||
| "release": "npm run prepare-release && node ./scripts/npm/release.js", | |||
| "size": "npm run build && size-limit" | |||
| "size": "npm run build-prod && size-limit" | |||
There was a problem hiding this comment.
Previously, the size measurements were done using the dev build! If we wanted to measure both dev and prod we would change this to build-release. If we use build-release here as-is it would take slightly longer and still use the prod build (through the fork module) so there's no reason to do that now unless we want to start measuring both.
Replace the
"webpack"condition in package.json exports with"development"and"production"to make it easier to skip consideration of the fork module. Before this change, there was no metadata that pointed directly to the dev or prod artifacts.See also: https://webpack.js.org/guides/package-exports/#optimizations
Not quite sure if rollup, vite, etc. support this condition without configuration but they have been demonstrated to work with the fork module as-is.
I was also thinking that this would be useful for CDN usage (as one would want with #5840) since esm.sh supports conditions (Development Mode)
I also noticed the size-limit action was failing so I added some custom configuration that I think should be easier to maintain. I also changed it to run build-prod instead of build so we are measuring the artifact that matters.