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: Improve css sourcemap accuracy #305

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

bfanger
Copy link
Contributor

@bfanger bfanger commented Apr 3, 2022

When using Vite's (experimental) css.devSourcemap feature
The filenames where correct, but the line numbers didn't match.

Changing:
source: filename
into
source: filename?.split(/[/\\]/).pop()

By changing source to only contain the filename instead of the full path fixed the line number issue.

Fixes #300

@dominikg
Copy link
Member

dominikg commented Apr 4, 2022

This is a great finding, thanks!

The PR needs to be rebased due to conflicts

@dominikg
Copy link
Member

dominikg commented Apr 4, 2022

some questions

  1. do we want to handle configuration automatically
    a) set for svelte when enabled in vite config
    b) set for vite when enabled in svelte config
    c) warn for conflicts?

  2. do we need to patch the output sourcemap either using https://github.com/vitejs/vite/blob/ace6e93239c85667eaea656cfda770e3269b336f/packages/vite/src/node/plugins/css.ts#L869 directly or some similar approaches.

@bfanger bfanger force-pushed the fix/css-sourcemaps branch 2 times, most recently from a1deba2 to 65f58e1 Compare April 4, 2022 21:13
@bfanger
Copy link
Contributor Author

bfanger commented Apr 4, 2022

Rebased and changed from split().pop() to using path.basename.

  1. Automatic Configuration
    This change does not impact any configuration for this plugin.
    I should mention that to activate the sourcemap feature you do need to
    add to your vite.config:
css: { devSourcemap: true }

or when using sveltekit to your svelte.config:

  kit: { 
    vite: {
      css: { devSourcemap: true }
    },
    ...

As you can see this a configuration option for vite, not the plugin.

  1. patching sourcemap?
    i think that step is already handled by svelte.preprocess & svelte.compile
    There might be some additional work needed for the svelte-preprocess package, but that's another repo.

I've also tested some sass using the experimental:{ useVitePreprocess: true }, and the generated sourcemap looked fine.

To debug i've used https://evanw.github.io/source-map-visualization/ and pasted the results i logged in packages/vite-plugin-svelte/src/utils/compile.ts using:

compiled.css.map && console.log(`\n${filename}\n\n${compiled.css.code}\n/*# sourceMappingURL=${compiled.css.map.toUrl()} */\n\n`) 

@dominikg
Copy link
Member

dominikg commented Apr 5, 2022

Sorry my comment above wasn't very clear.

I should mention that to activate the sourcemap feature you do need to
add to your vite.config:

css: { devSourcemap: true }

vite-plugin-svelte can set devSourcemap: true automatically in the config hook so the user doesn't have to do it. This would work for both vite+svelte and sveltekit ootb.

It could be added here:

unfortunately there are 2 options svelte.compilerOptions.enableSourcemap.css and vite.css.devSourcemap so if either of them is false in user config, the other should be too.

@bfanger
Copy link
Contributor Author

bfanger commented Apr 5, 2022

I would not auto enable it just yet, i've seen this experimental feature throw javascript exceptions during build/dev.
(with an older complex sass setup)

I'd assume vite will enable the feature by default at some point.

@dominikg
Copy link
Member

dominikg commented Apr 5, 2022

I would not auto enable it just yet, i've seen this experimental feature throw javascript exceptions during build/dev. (with an older complex sass setup)

I'd assume vite will enable the feature by default at some point.

we can handle the config later in a separate PR, true. although i do lean on early adpoting things that improve the dev experience. and missing css sourcemaps has been a sore spot.

@dominikg
Copy link
Member

dominikg commented Apr 5, 2022

final step missing before merge would be a changeset pnpm changeset create a patch for vite-plugin-svelte with a small note

@bfanger
Copy link
Contributor Author

bfanger commented Apr 5, 2022

👍 done!

@dominikg dominikg merged commit d57bd70 into sveltejs:main Apr 5, 2022
@github-actions github-actions bot mentioned this pull request Apr 5, 2022
@dominikg
Copy link
Member

dominikg commented Apr 6, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

css sourcemaps
3 participants