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

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

Closed
wants to merge 1 commit into from

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