CSS modules: remove unused classes #5363
Merged
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
|
This pull request has been linked to and will mark 1 task as "Done" when merged:
|
mischnic
force-pushed
the
css-modules-tree-shaking
branch
from
95a8834
to
072ab5b
Compare
mischnic
commented
Nov 19, 2020
Benchmark ResultsKitchen Sink ✅
Timings
Cold Bundles
Cached Bundles
React HackerNews ✅
Timings
Cold Bundles
Cached Bundles
AtlasKit Editor ✅
Timings
Cold Bundles
Cached Bundles
Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
mischnic
force-pushed
the
css-modules-tree-shaking
branch
from
bd43503
to
3fb781d
Compare
mischnic
force-pushed
the
css-modules-tree-shaking
branch
3 times, most recently
from
8fea0fd
to
cae5b33
Compare
mischnic
commented
Dec 15, 2020
| @@ -36,8 +36,8 @@ | |||
| "parcel-bundler": "2.0.0-beta.1", | |||
| "parcel": "2.0.0-beta.1", | |||
| "postcss-custom-properties": "^8.0.9", | |||
| "postcss-import": "^12.0.1", | |||
| "postcss": "^8.0.0", | |||
| "postcss-import": "^13.0.0", | |||
There was a problem hiding this comment.
One interesting situation that I ran into was that the old postcss-import version didn't use PostCSS 8 and inserted nodes into the AST that were missing some methods from recent my PostCSS PR (toJSON). This caused a build error
mischnic
force-pushed
the
css-modules-tree-shaking
branch
from
cae5b33
to
b7fde05
Compare
devongovett
reviewed
Dec 16, 2020
| [...asset.symbols].map(([, {local}]) => `.${local}`), | ||
| ); | ||
| let usedLocalSymbols = | ||
| // we have to still support the more common default imports |
There was a problem hiding this comment.
Wonder if we should add a warning here so people know to switch from default imports to namespaces?
There was a problem hiding this comment.
A warning would be nice, but at this point, there is no way to differentiate a default import from this case:
import * as styles from "..."
console.log(styles.default);Which also means that calling a class default means no class gets shaken....
There was a problem hiding this comment.
Can we add some kind of metadata to the dependency to mark this? I think a lot of projects might not see any benefit from this because they use default imports...
mischnic
force-pushed
the
css-modules-tree-shaking
branch
from
b7fde05
to
00af5ea
Compare
mischnic
force-pushed
the
css-modules-tree-shaking
branch
from
00af5ea
to
0572d04
Compare
mischnic
force-pushed
the
css-modules-tree-shaking
branch
from
0572d04
to
4c693b0
Compare
devongovett
approved these changes
Jan 1, 2021
| (asset.value.env.scopeHoist && asset.value.type === 'js') || | ||
| (this.options.mode === 'production' && | ||
| asset.value.type === 'css' && | ||
| asset.value.symbols) |
| let loc = defaultImport.symbols.get('default')?.loc; | ||
| logger.warn({ | ||
| message: | ||
| 'CSS modules cannot be tree shaken when imported with a default specifier', |
There was a problem hiding this comment.
We could get rid of this limitation if we tracked member accesses of default/named imports like we were just discussing 😉
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.
Depends on #5362Waiting for postcss/postcss#1484Closes T-783
Example usage (this has to be a namespace import, not a default import!):