Skip to content

Comments

fix(css): convert map returned by vite:css transform to SourceMap (fixes #4939)#4950

Closed
dominikg wants to merge 1 commit intovitejs:mainfrom
dominikg:fix/css-sourcemap-types
Closed

fix(css): convert map returned by vite:css transform to SourceMap (fixes #4939)#4950
dominikg wants to merge 1 commit intovitejs:mainfrom
dominikg:fix/css-sourcemap-types

Conversation

@dominikg
Copy link
Contributor

@dominikg dominikg commented Sep 16, 2021

(fixes #4939)

Description

the map returned by postcss can be a SourceMapGenerator. Use a function to check and call toJSON() on it.
additionally throw an error if the returned map is not something we can pass on as part of the transform result.

An alternate solution would be to roll back the offending commit as this change would not lead to satisfactory sourcemap support on it's own and getting it done isn't a short term endeavor

Additional context

I wasn't able to come up with a testcase for this due to that fact that this map isn't turning up in any results. vite css sourcemaps are completely broken at the moment, especially for bundled and minified output a lot of things need to be added.

  1. capture and merge maps in css bundling
  2. pass on sourcemap option to esbuild minifier (careful, esbuild has different options, eg. no "hidden")
  3. output css maps on dev somehow, maybe even for css turned to js modules?
  4. ... ?

there is an old enhancement ticket about it #2830 maybe it's time to revisit that someday


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.

@patak-dev
Copy link
Member

@dominikg thanks for digging into this. We are aiming to start the beta period for 2.6 (probably next Monday). And then there will be likely a week for the community to test it. If you think you or others can work on this to try to support it and use the beta for testing, I think it is good. If not, IMO better we revert and do this more calmly for 2.7

@dominikg
Copy link
Contributor Author

revert PR is here: #4951

@dominikg dominikg marked this pull request as draft September 16, 2021 15:07
@Shinigami92 Shinigami92 added p3-minor-bug An edge case that only affects very specific usage (priority) feat: css labels Sep 16, 2021
@Shinigami92 Shinigami92 self-requested a review September 16, 2021 16:08
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

None yet

Development

Successfully merging this pull request may close these issues.

Sourcemap is likely to be incorrect: a plugin (undefined) was used to transform files...

3 participants