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(css): support css module compose/from path resolution #4378

Merged
merged 4 commits into from
Jul 31, 2021

Conversation

kamilic
Copy link
Contributor

@kamilic kamilic commented Jul 24, 2021

Description

Vite don't support path resolving when using compose / from on cssmodule file currently because postcss-modules didn't provide options liked resolve in postcss-import.

And now postcss-modules supported path resolutiion feature on 4.2.2 so that we can implement / fix this feature.

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 Commit 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.

@kamilic kamilic marked this pull request as ready for review July 24, 2021 09:20
@Shinigami92 Shinigami92 added feat: css p2-nice-to-have Not breaking anything but nice to have (priority) labels Jul 24, 2021
@Shinigami92 Shinigami92 changed the title feat(css.ts): support cssmodule compose/from path resolution feat(css): support css module compose/from path resolution Jul 24, 2021
@Shinigami92
Copy link
Member

Is it possible to add a test case?

As you can see, there is also an error you need to fix:

src/node/plugins/css.ts(670,36): error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'CSSAtImportResolvers'.

@kamilic
Copy link
Contributor Author

kamilic commented Jul 25, 2021

Is it possible to add a test case?

As you can see, there is also an error you need to fix:

src/node/plugins/css.ts(670,36): error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'CSSAtImportResolvers'.

ok, I will try to add some test case and fix errors.

@kamilic kamilic force-pushed the feature-resolve-module-css-path branch from 9823e2b to 66463ff Compare July 25, 2021 04:48
@kamilic kamilic force-pushed the feature-resolve-module-css-path branch from a0df1f8 to e3692f5 Compare July 25, 2021 07:04
@kamilic
Copy link
Contributor Author

kamilic commented Jul 25, 2021

@Shinigami92 I added a .css cssmodule file test case only, but it support sass and less file path resolution actually.
I am not sure what is the best way to write tests in these situations instead of copy the css test case again and again.

Shinigami92
Shinigami92 previously approved these changes Jul 25, 2021
@patak-dev
Copy link
Member

Thanks for the PR @kamilic. The change looks good to me. About the added test, I think it would be better to add it as an integration test is the css playground. These tests are better for us because we can catch wrong interactions between the diferent pieces. Would you check if it is possible to move it to packages/playground/css?

@kamilic
Copy link
Contributor Author

kamilic commented Jul 26, 2021

Thanks for the PR @kamilic. The change looks good to me. About the added test, I think it would be better to add it as an integration test is the css playground. These tests are better for us because we can catch wrong interactions between the diferent pieces. Would you check if it is possible to move it to packages/playground/css?

I will find time to add some test on playground.

@kamilic
Copy link
Contributor Author

kamilic commented Jul 31, 2021

@patak-js playground tests now added.
And an anothor problem founded. HMR seems not working when I change content of the composed cssmodule file. The composed .module.css file seems not to be considered as a dependency.

I think it should be fixed on other pull requests so I commented out some test code and add a @todo tag.

@patak-dev
Copy link
Member

Great! Thanks for taking the time to add the tests.

About the new issue, it would be good to also create a bug report with a that reproduction against the current version so it easier to track it.

@underfin underfin merged commit 690b35e into vitejs:main Jul 31, 2021
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants