Skip to content
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

feat!: if not processed, CSS Modules return a proxy, scope class names by filename #1803

Merged
merged 7 commits into from
Sep 4, 2022

Conversation

sheremet-va
Copy link
Member

@sheremet-va sheremet-va commented Aug 5, 2022

Closes #1512

This PR implements the following logic:

  • By default, CSS processing is disabled for every css file
    • module.* exports a proxy in that case to not affect runtime (meaning, styles.{value} will always return a key with filename hash)
  • If processing is enabled (css.include), css is processed and injected into DOM
    • By default, module.* returns pseudo-scoped class names (styles.module always returns _module_${filenameHash}, styles.someRandomValue will return undefined)
    • If you want to enable original scoped names (so production and tests generate the same names), use css.modules.classNameStrategy: 'scoped'
  • Added css.modules.classNameStrategy option:
    • scoped - use css.modules.generateScopeName from user config or fallback to default behaviour, this is how it works in dev and prod
    • non-scoped - returns just the name
    • stable - returns the nama and hash, based on filepath, relative to root

@sheremet-va sheremet-va marked this pull request as ready for review September 3, 2022 13:02
@sheremet-va sheremet-va changed the title feat: add option to keep class names for css modules feat: when not processed, CSS Modules return a proxy, do not scope CSS Modules class names by default Sep 3, 2022
docs/config/index.md Outdated Show resolved Hide resolved
@Demivan
Copy link
Member

Demivan commented Sep 3, 2022

How about making generateScopedName depend on classname and filename? This should should prevent class name conflict issues and make it so generated class names don't change when css content changes.

This is what postcss-modules uses in their tests.
https://github.com/madyankin/postcss-modules/blob/a400e145e0ae9e996e51eed81a2e8e2a1f604527/test/test.js#L31-L34

@sheremet-va
Copy link
Member Author

sheremet-va commented Sep 3, 2022

How about making generateScopedName depend on classname and filename? This should should prevent class name conflict issues and make it so generated class names don't change when css content changes.

This is what postcss-modules uses in their tests. madyankin/postcss-modules@a400e14/test/test.js#L31-L34

They are using basename, so it can still cause conflicts, if you have /foo/index.css and /baz/index.css for example. We can encode the whole path to fix this, of course, generating _${className}_${encodedFilepath} (_main_32rsfdr3, it won't change when content changes, but will change, if filename is changed, or file is moved to other directory) @isaec, what do you think?

This also changes semantics and requires different naming.

@sheremet-va sheremet-va changed the title feat: when not processed, CSS Modules return a proxy, do not scope CSS Modules class names by default feat!: when not processed, CSS Modules return a proxy, do not scope CSS Modules class names by default Sep 3, 2022
@Demivan
Copy link
Member

Demivan commented Sep 3, 2022

This also changes semantics and requires different naming.

Maybe enum css.modules.classNames:

  • stable - (default) class name + hashed path
  • scoped - original implementation
  • raw?, non-scoped?, source? - source class names

@a-viv-a
Copy link

a-viv-a commented Sep 3, 2022

How about making generateScopedName depend on classname and filename? This should should prevent class name conflict issues and make it so generated class names don't change when css content changes.
This is what postcss-modules uses in their tests. madyankin/postcss-modules@a400e14/test/test.js#L31-L34

They are using basename, so it can still cause conflicts, if you have /foo/index.css and /baz/index.css for example. We can encode the whole path to fix this, of course, generating _${className}_${encodedFilepath} (_main_32rsfdr3, it won't change when content changes, but will change, if filename is changed, or file is moved to other directory) @isaec, what do you think?

This also changes semantics and requires different naming.

For my uses, the stability was the desired characteristic. If importing the stylesheet has the correct property string mapping, then stable or source both work for me. Stable seems like a good default, still discouraging selector matching for tests (unless you import the style object), but not providing unneeded noise in snapshot testing.

@sheremet-va
Copy link
Member Author

Option scopeClassNames is removed, option classNameStrategy is added with options:

  • scoped - use css.modules.generateScopeName from user config or fallback to default behaviour, this is how it works in dev and prod
  • non-scoped - returns just the name
  • stable - returns the nama and hash, based on filepath, relative to root

@antfu
Copy link
Member

antfu commented Sep 4, 2022

I am not familiar with CSS modules, so I'd trust your judgment here. Feel free to merge once you meet the agreement :)

@sheremet-va sheremet-va changed the title feat!: when not processed, CSS Modules return a proxy, do not scope CSS Modules class names by default feat!: if not processed, CSS Modules return a proxy, scope class names by filename Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically mock css module imports
4 participants