Skip to content

fix(css modules): apply preprocessor to composed file if needed (fix #10340)#12076

Open
Thiry1 wants to merge 3 commits intovitejs:mainfrom
Thiry1:fix/10340
Open

fix(css modules): apply preprocessor to composed file if needed (fix #10340)#12076
Thiry1 wants to merge 3 commits intovitejs:mainfrom
Thiry1:fix/10340

Conversation

@Thiry1
Copy link

@Thiry1 Thiry1 commented Feb 16, 2023

Description

apply preprocessor to composed file if needed.
fixes: #10340

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Thiry1 Thiry1 marked this pull request as ready for review February 16, 2023 12:47

if (isModule) {
const FileSystemLoader = // @ts-expect-error TODO: needs types
(await import('postcss-modules/build/FileSystemLoader')).default
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +940 to +944
async fetch(
_newPath: string,
relativeTo: string,
_trace: string,
): Promise<any> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Niputi Niputi added feat: css p3-minor-bug An edge case that only affects very specific usage (priority) labels Apr 14, 2023
@jeron-diovis
Copy link

jeron-diovis commented Jul 15, 2023

I built this branch and installed it into my project.
Can confirm that main usecase does work: using composes on .scss file, containing line-comments and mixins, doesn't cause any errors, and resulting styles working as expected.

Importing values via @value directive from .scss file does work too.

Using composes on .css file containing nested selectors (powered by postcss-nested plugin) does work too.


More sophisticated cases which don't work (idk whether they should):

  • using composes on .css file containing mixins (via postcss-mixins plugin): no errors produced, but mixins are not processed. Output styles is like this:
._mix_css_1n1sl_5 {
  @mixin test: ;
}
  • using composes on .css file containing line-comments (with syntax: postcss-scss option enabled in postcss config): causes build crash: CssSyntaxError: [...]/css.module.css:1:1: Unexpected '/'. Escaping special characters with \ may help.

@sapphi-red
Copy link
Member

Thanks for the PR. We discussed about this PR in the last meeting. We think we can have this feature. The remaining things that this PR needs is:

  1. Someone needs to check how to handle the sophisticated cases described in the comment above (fix(css modules): apply preprocessor to composed file if needed (fix #10340) #12076 (comment))
  2. Avoid using the internals of postcss-modules (e.g. postcss-modules/build/FileSystemLoader)
  3. Check if this can be supported by lightningcss as well in future
  4. Add source map support

@patak-cat
Copy link
Member

@Sec32fun32 we'll need to ban you from the org if you continue sending these approvals, they are generating notifications for everyone in watching the repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat: css p3-minor-bug An edge case that only affects very specific usage (priority)

Projects

Status: Changes Requested

Development

Successfully merging this pull request may close these issues.

[css modules]composed scss file is treated as css

5 participants