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] ssr remove css map #6744

Closed
wants to merge 1 commit into from
Closed

[fix] ssr remove css map #6744

wants to merge 1 commit into from

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Sep 18, 2021

Fixes sveltejs/kit#720.

The inline sourcemap's sourcesContent was interfering with Vite's production replacement, since they are applied near the end of the build process (after svelte has been compiled). So we remove the sourcemap instead since it wasn't actually being used.

REPL to show css map issue. (Compile to ssr)

See sveltejs/kit#720 (comment) for more info.

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

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@Conduitry
Copy link
Member

This makes me nervous. Is there anything else this might be breaking, even if other official Svelte tooling doesn't use this? Do browsers use these CSS sourcemaps?

@benmccann

This comment has been minimized.

@bluwy
Copy link
Member Author

bluwy commented Sep 23, 2021

This makes me nervous. Is there anything else this might be breaking, even if other official Svelte tooling doesn't use this? Do browsers use these CSS sourcemaps?

There was a thread about it in the maintainers chat. But in short, the only way someone could access the sourcemap is when there's a bundler plugin that extracts the inline css sourcemap after compiling the svelte component in SSR. And I can't think of a reason someone wants to do that, especially that the official way to get the css sourcemap is via App.render().css.map (which is currently null, unimplemented).

AFAIK, the sourcemap can't be leaked in any other ways than mentioned above. And if the generated code isn't following semver, it should be safe to make the change.

I think vitejs/vite#4192 might be a better solution to this issue. Perhaps we could convince @GrygrFlzr to revive it at some point?

That PR won't help with this though since it would still do code replacements and mess with the code in the css sourcemap.

@bluwy
Copy link
Member Author

bluwy commented Sep 25, 2021

Closing in favour of a new options.sourcemap option of boolean | { js: boolean; css: boolean }. If css is false, we remove the css map. For vite-plugin-svelte, we can force options.sourcemap.css false.

@bluwy bluwy closed this Sep 25, 2021
@bluwy
Copy link
Member Author

bluwy commented Sep 28, 2021

Tried making the options.sourcemap feature today, but I realized there's already an existing options.sourcemap implementation, which accepts a type of string | object (for actual sourcemap object). So the plan wouldn't really work unless we create new properties for them.

I'm thinking something like options.enableSourcemap: 'js' | 'css' | 'all' | 'none', or options.cssSourcemap: boolean could work. But it still feels somewhat awkward.

@benmccann
Copy link
Member

Yeah, it looks like that option was added in #5584, but not documented. We should probably add it to the documentation.

Regarding the API for the new option, I wonder if just on / off is enough. E.g. Rollup lets you choose between inline, separate file, hidden (which I don't quite understand the description of), or off: https://rollupjs.org/guide/en/#outputsourcemap

@bluwy
Copy link
Member Author

bluwy commented Oct 11, 2021

I'm not sure if on/off could work since in SSR we might want to have JS sourcemaps, but no CSS sourcemaps. Currently in SSR, CSS sourcemaps are always inlined by Svelte, even before the bundler step, which feels a bit weird but I'm not familiar with sourcemaps enough to justify. I might go with the options.enableSourcemap: 'js' | 'css' | 'all' | 'none' idea in the coming days (with a lot of implementation uncertainty), but if it works it would be a nice perf improvement for Svelte too.

@benmccann
Copy link
Member

I was saying that even for js vs css just turning them on vs off might not be enough because maybe you'd need to decide inline vs external for each one. But actually, I'm not sure my thought was correct because now I'm remembering that I think the idea is that the plugins pass along the sourcemap via the Rollup API and then it's Rollup that would decide inline vs external at the end after collecting and merging all the source maps (I think)

This isn't a problem on the client-side because the CSS is extracted out into separate files. I wonder if we could do that on the server-side as well. Though that might have to be a v4 thing if we did it since I think it'd be a breaking change

@bluwy
Copy link
Member Author

bluwy commented Oct 11, 2021

Ah gotcha. Yeah I too haven't wrapped my head around how we would handle sourcemaps in the server-side. In the meantime, I'll try to get an simple but agnostic option to remove this specific sourcemap use-case.

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.

Environment variables in Svelte templates don't work with npm run build
3 participants