css modules: use a terser selector format that requires less escaping #3437
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
9 Ignored Deployments
|
Benchmark for 6f304eeClick to view benchmark
|
wbinnssmith
force-pushed
the
wbinnssmith/css-modules-simplify-classname
branch
from
b18df50
to
81a23e4
Compare
🟢 CI successful 🟢Thanks |
wbinnssmith
commented
Jan 23, 2023
| @@ -31,3 +31,8 @@ turbopack-env = { path = "../turbopack-env" } | |||
|
|
|||
| [build-dependencies] | |||
| turbo-tasks-build = { path = "../turbo-tasks-build" } | |||
|
|
|||
| [features] | |||
There was a problem hiding this comment.
Added this section so cargo test -p turbopack-tests is possible. cc @kwonoj
Benchmark for ba33241Click to view benchmark
|
wbinnssmith
force-pushed
the
wbinnssmith/css-modules-simplify-classname
branch
from
075a54c
to
0a13e5c
Compare
Benchmark for 81c90e8
Click to view full benchmark
|
wbinnssmith
force-pushed
the
wbinnssmith/css-modules-simplify-classname
branch
from
0a13e5c
to
242196a
Compare
jridgewell
approved these changes
Jan 24, 2023
Benchmark for 42ba4a0Click to view benchmark
|
ForsakenHarmony
approved these changes
Jan 24, 2023
sokra
requested changes
Jan 24, 2023
There was a problem hiding this comment.
I don't like this change. Before it was obvious from a class name from which source file it comes from. Now this is no longer the case.
I don't see the benefit of reading generated code, since the user will never read it anyway. It's source mapped and links to the source will point to the original css. Instead they might read the html or Dom where escaping is not required.
But I see the escaping problem on front. How about using the current content but converting it to a escape free format e.g. by using underscores, or using our magic identifier encoding
|
@sokra readability of the classes in the HTML tree is terrible when there are multiple classes applied to one element maybe we can add some configuration |
|
I agree with making the change to a smaller classname.
The users see generated code in the DOM tree. Having such long classnames is overwhelming. And the ability to associate a classname with the file is still there, you just need to click the classname int the style panel to open up the defining file. |
sokra
approved these changes
Jan 31, 2023
wbinnssmith
added a commit
that referenced
this pull request
Feb 1, 2023
#3437 wasn't up to date with main when it was merged. Test Plan: CI
mehulkar
pushed a commit
that referenced
this pull request
Feb 1, 2023
#3437 wasn't up to date with main when it was merged. Test Plan: CI
timneutkens
pushed a commit
to vercel/next.js
that referenced
this pull request
Feb 10, 2023
…49736102+kodiakhq[bot]@users.noreply.github.com> # New Features - vercel/turbo#3540 - vercel/turbo#3549 - vercel/turbo#3465 - vercel/turbo#3550 - vercel/turbo#3495 - vercel/turbo#3624 - vercel/turbo#3600 - vercel/turbo#3676 - vercel/turbo#3689 # Fixes - vercel/turbo#3437 - vercel/turbo#3542 - vercel/turbo#3531 - vercel/turbo#3552 - vercel/turbo#3551 - vercel/turbo#3597 - vercel/turbo#3644 - vercel/turbo#3623 - vercel/turbo#3634 - vercel/turbo#3574 - vercel/turbo#3673 - vercel/turbo#3675 - vercel/turbo#3723 - vercel/turbo#3677 - vercel/turbo#3717 - vercel/turbo#3701 # Performance Improvements - vercel/turbo#3361 - vercel/turbo#3619 --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
wbinnssmith
added a commit
to vercel/next.js
that referenced
this pull request
Mar 1, 2023
Turbopack uses a different format for its css module classnames [0]. This adds and uses a helper function to the next-font tests to assert against these classnames when testing with Turbopack. Ideally these tests could be more behavioral (e.g. asserting on rendered dimensions), but these can't capture things like fallback fonts. [0] vercel/turbo#3437
timneutkens
pushed a commit
to vercel/next.js
that referenced
this pull request
Mar 6, 2023
Turbopack uses a different format for its css module classnames [0]. This adds and uses a helper function to the next-font tests to assert against these classnames when testing with Turbopack. Ideally these tests could be more behavioral (e.g. asserting on rendered dimensions), but these can't capture things like fallback fonts. [0] vercel/turbo#3437
jridgewell
pushed a commit
to vercel/next.js
that referenced
this pull request
Mar 10, 2023
…vercel/turbo#3437) Fixes WEB-447. Previously, generated classnames would include literal `/` characters, which need escaping when generating selectors. While users should still escape selectors, this avoids it for this case and makes generated code easier to read. This format is also more aligned with what webpack-based css modules generates. This also removes the square character as a separator, since it breaks words and makes double-clicking for selection more difficult.
sokra
pushed a commit
to vercel/next.js
that referenced
this pull request
Mar 13, 2023
…vercel/turbo#3437) Fixes WEB-447. Previously, generated classnames would include literal `/` characters, which need escaping when generating selectors. While users should still escape selectors, this avoids it for this case and makes generated code easier to read. This format is also more aligned with what webpack-based css modules generates. This also removes the square character as a separator, since it breaks words and makes double-clicking for selection more difficult.
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.
Fixes WEB-447.
Previously, generated classnames would include literal
/characters, which need escaping when generating selectors. While users should still escape selectors, this avoids it for this case and makes generated code easier to read. This format is also more aligned with what webpack-based css modules generates.This also removes the square character as a separator, since it breaks words and makes double-clicking for selection more difficult.