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

Revert "[fix] add filename to combined source map if needed (#6089)" #6572

Closed
wants to merge 1 commit into from
Closed

Revert "[fix] add filename to combined source map if needed (#6089)" #6572

wants to merge 1 commit into from

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Jul 25, 2021

This reverts commit 4ca2af4.

Fixes sveltejs/kit#2029

@dominikg
Copy link
Member

the og. PR contained more commits, should they also be reverted?
https://github.com/sveltejs/svelte/pull/6089/commits

@benmccann
Copy link
Member Author

benmccann commented Jul 25, 2021

There's only a single commit after being squashed. It's a bit weird because the PR being reverted (#6089) and #6427 both fixed the same issue in different ways and so both updated the same set of tests. So this PR doesn't revert most of the tests since the test updates were also merged as part of #6427

@dominikg
Copy link
Member

for the record, this PR in vite: vitejs/vite#2904 (merged but unreleased as of typing) might fix it too.

@benmccann
Copy link
Member Author

Ahh! That would explain then why all the reports are coming in just for SvelteKit. They said they're doing a new release of Vite tomorrow, so maybe we should hold off on this PR then and see what the behavior is with the new version of Vite. It sounds like it may print a warning then which we can investigate to see what the issue is

@dominikg
Copy link
Member

vite 2.4.4 prints a warning instead of the ENOENT error.

sveltejs/kit#2001 (comment)

@Conduitry Conduitry closed this Jul 27, 2021
@benmccann benmccann reopened this Jul 29, 2021
@benmccann benmccann marked this pull request as ready for review July 29, 2021 17:44
@benmccann benmccann closed this Jul 29, 2021
@benmccann benmccann reopened this Jul 29, 2021
@benmccann
Copy link
Member Author

I still think we should move ahead with this PR to fix the warnings people are getting with SvelteKit. Vite folks said the source maps being added here were wrong - they should be relative to output file and not relative to project root: vitejs/vite#4423 (comment). There's not as strong a need for this change since #6427 as committed

@dominikg
Copy link
Member

here's the spec https://sourcemaps.info/spec.html#h.75yo6yoyk7x5

this could be fixed without reverting by setting a relative source then? either by passing in a different filename from vite-plugin-svelte or stripping the path before setting source.

@benmccann
Copy link
Member Author

Yes, it could be fixed by setting a relative source. I'm not sure it's a priority though. I could definitely find higher ROI things to work on

@dominikg
Copy link
Member

This seems to be a problem within compile itself, the change for preprocess just uncovered it:

  1. sourceMapSource is set to filename here, should be a relative path

    sourceMapSource: compile_options.filename

  2. a relative path is set here if compileOptions.outoutFilename is set, but it's keeping the absolute path if not

    compile_options.filename ? get_relative_path(compile_options.outputFilename || '', compile_options.filename) : null

  3. this.file is passed to apply_preprocessor_sourcemap here, again using the file path instead of the relative one

    js.map = apply_preprocessor_sourcemap(this.file, js.map, compile_options.sourcemap as (string | RawSourceMap | DecodedSourceMap));

I think this can be solved by calculating the correct relative path (just the filename minus path unless outputFilename is set and a different basepath) once and then using that in all 3 places mentioned above.

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.

Sourcemap for "/.../...svelte" points to missing source files (if empty script tag)
3 participants