Improve treeshakeability of build artifacts #721
Draft
+449
−581
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for reselect-docs canceled.
|
|
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. |
- It seems like `webpack4` prefers the `browser` field over the `module` field, and the current build artifact seems to be incompatible with it so we are going to remove it for now.
markerikson
approved these changes
Jun 1, 2024
| @@ -2,15 +2,15 @@ | |||
| "name": "reselect", | |||
| "version": "5.1.0", | |||
| "description": "Selectors for Redux.", | |||
| "main": "./dist/cjs/reselect.cjs", | |||
There was a problem hiding this comment.
❓ What's the bundle size change from having separate prod/dev CJS artifacts here?
There was a problem hiding this comment.
Can't tell right now. I'll be home soon and I can give you some numbers if you want but from what I remember it was positive, that's why I did it.
| @@ -115,7 +111,7 @@ jobs: | |||
|
|
|||
| # Note: We currently expect "FalseCJS" failures for Node16 + `moduleResolution: "node16", | |||
| - name: Run are-the-types-wrong | |||
| run: npx @arethetypeswrong/cli ./package.tgz --format table --ignore-rules false-cjs | |||
| run: npx @arethetypeswrong/cli@latest ./package.tgz --format table --ignore-rules false-esm | |||
There was a problem hiding this comment.
Now we have reselect.d.mts because of tsup version 8.
| @@ -10,11 +10,11 @@ import { | |||
| dirtyTag | |||
| } from './tracking' | |||
|
|
|||
| export const REDUX_PROXY_LABEL = Symbol() | |||
| export const REDUX_PROXY_LABEL = /* @__PURE__ */ Symbol() | |||
There was a problem hiding this comment.
❓ does the annotation actually do something here?
There was a problem hiding this comment.
Yes. That symbol somehow stays around even if it's not used. The pure annotation removes it. It's the case with both rollup and webpack.
|
Anything else you want to add here? Also, what's the net bundle size changes? (suppose it might help if we merge the |
|
Yeah let's do the size limit one first. Do you want me to get rid of the browser artifact? |
|
I think I'm missing some context here. I see that you added and removed a |
|
The CI example app for cra4 failed when I added it. Edit: The cra4 example app failed for React-Redux when I added it, it failed because it has a |
…-treeshakeability
- `env` is the same thing as `define` with `JSON.stringify`.
- Currently there is no difference between emitting type definitions with `cjs` or `esm` format, the only difference is the file extension. So we emit the type definitions with a `cjs` format to preserve the previous `.d.ts` extension as opposed to the new `.d.mts` extension.
- Currently there is no difference between emitting type definitions with `cjs` or `esm` format, the only difference is the file extension. So we emit the type definitions with a `cjs` format to preserve the previous `.d.ts` extension as opposed to the new `.d.mts` extension.
…-treeshakeability
aryaemami59
force-pushed
the
fix-treeshakeability
branch
from
133c303
to
1520b44
Compare
Other changes to consider adding:
"exports": {
"./package.json": "./package.json",
".": {
"types": "./dist/reselect.d.ts",
"import": "./dist/reselect.mjs",
"default": {
"development": "./dist/cjs/reselect.development.cjs",
"production": "./dist/cjs/reselect.production.min.cjs",
"default": "./dist/cjs/index.js"
}
}
},
"exports": {
"./package.json": "./package.json",
".": {
"types": "./dist/reselect.d.mts",
"bun": "./src/index.ts",
"import": "./dist/reselect.mjs",
"default": "./dist/cjs/index.js"
}
},
"exports": {
"./package.json": "./package.json",
".": {
"import": {
"types": "./dist/reselect.d.mts",
"default": "./dist/reselect.mjs"
},
"default": {
"types": "./dist/cjs/reselect.d.ts",
"default": "./dist/cjs/index.js"
}
}
},Or maybe even: "exports": {
"./package.json": "./package.json",
".": {
"types": {
"import": "./dist/reselect.d.mts",
"default": "./dist/reselect.d.ts"
},
"import": "./dist/reselect.mjs",
"default": "./dist/cjs/index.js"
}
}, |
|
Can you summarize for me:
|
SummaryWith Rollup
|
|
Nice, that does seem like it's better! |
|
@markerikson I'll try to put togther the same kind of chart for the other Redux repos as well. It might be good to have it as a point of reference. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR:
tsupconfig to add production and development CJS output files.@__PURE__annotation to some of the function call sites to improve treeshakeability.WeakRefto a function calledgetWeakRefwith@__PURE__annotation to its call site.browserfield inpackage.jsonto browser artifact. (Not sure if we want to keep thebrowserartifact so if not let me know and I can get remove it).