Skip to content

Consume and generate sourcemaps#1863

Closed
RedHatter wants to merge 5 commits into
sveltejs:masterfrom
RedHatter:sourcemaps
Closed

Consume and generate sourcemaps#1863
RedHatter wants to merge 5 commits into
sveltejs:masterfrom
RedHatter:sourcemaps

Conversation

@RedHatter

@RedHatter RedHatter commented Nov 24, 2018

Copy link
Copy Markdown
Contributor

preprocess composes any sourcemaps the preprocessors generate and returns them through a getMap function.

compile can be provided a sourcemap to consume through a new option sourceMap.

I tried to update source-map to the latest version but they moved to a promise based api which causes problems with our synchronous compile function.

As far as I can tell it works correctly but test need to be written.

@RedHatter

RedHatter commented Dec 2, 2018

Copy link
Copy Markdown
Contributor Author

I'm not quite sure how to handle relative vs absolute paths. Svelte will simply use the path passed in as the source path. This can cause problems when the compiler is passed an absolute path (rollup) and a sourcemap with relative paths (I think that's correct?). There a few ways to deal with this.

  1. Do nothing. Let the caller of the compiler make sure that the paths match.
  2. Internally convert the file path to relative.
    2 b. Internally convert the souremap paths to relative as well to guarantee they match.
  3. Internally convert the souremap paths to absolute.

Currently number two is implemented but I'm not completely sure it's the right approach. Maybe someone with more experience with sourcemaps can weigh in? Are absolute paths even allowed in source maps?

@Conduitry

Copy link
Copy Markdown
Member

Hi, thanks so much for the PR, and sorry for the lack of replies while most of the attention was on Svelte v3 stuff!

As I mentioned in chat leading up to this PR, I still feel that the compiler shouldn't do anything with trying to turn absolute paths into relative ones. The compiler is currently completely blind to the filesystem (and is able to happily run in the browser in the REPL), which would be complicated by the use of process.cwd. This really feels to me something that ought to be some other library's responsibility if we can't do it purely from what the compiler is passed.

A more concrete problem is that this now has merge conflicts with master. I'll take a look at that sometime soon and try to resolve them if no one gets to it first.

@shirotech

Copy link
Copy Markdown

+1, this is rather a very useful feature. Would be great if this can be looked at. I can do another PR with rebased from master if it helps?

@Rich-Harris

Copy link
Copy Markdown
Member

Yeah, I totally dropped the ball on this — sorry. @shirotech that would be amazing, if you're still able to. We'd need some tests along with the PR, and it'd be great if we could avoid using source-map (I have some concerns about the package that haven't been addressed, so ideally we wouldn't need to bundle it alongside the compiler). Thank you!

@halfnelson

Copy link
Copy Markdown
Contributor

this uses source-map 0.6.1 which is the last of the sync versions of source-map, the rest are async due to wasm module loading. I couldn't find a nicer source map tool when I was looking for my own purposes. @Rich-Harris is a version that uses 0.6.1 of source-map still an option?

@shirotech

Copy link
Copy Markdown

@Rich-Harris

I think we can continue to use the source-map package if the main concern is the whatwg-url polyfill.

This can easily be addressed with overriding the package with an alternative using the resolutions field from the package.json.

e.g. in package.json

  "resolutions": {
    "whatwg-url": "npm:url-shim"
  }

What are your thoughts on this? I can do a rebase and we can remove the resolutions once or if mozilla/source-map#413 has been merged.

@Conduitry

Copy link
Copy Markdown
Member

I think also of concern was later versions of source-map being asynchronous.

@shirotech

Copy link
Copy Markdown

I’m not sure if that will impact anything, will changing the method signature to be async if needed cause any issues other than some refactoring?

@Conduitry

Copy link
Copy Markdown
Member

The Svelte compiler is itself currently synchronous, which is something we'd like to preserve for a number of reasons. Using an asynchronous library during compilation would make the compiler asynchronous which would break a bunch of existing tooling. The ESLint plugin, for example, would no longer be able to work at all, since ESLint plugins need to operate synchronously on the input code.

@antony

antony commented Jul 11, 2020

Copy link
Copy Markdown
Member

Closing in favour of #5015

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.

6 participants