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

Consume and generate sourcemaps #1863

Closed
wants to merge 5 commits into from

Conversation

RedHatter
Copy link
Contributor

@RedHatter RedHatter commented Nov 24, 2018

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
Copy link
Contributor Author

RedHatter commented Dec 2, 2018

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
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

+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
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
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

@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
Member

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

@shirotech
Copy link

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
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
Copy link
Member

antony commented Jul 11, 2020

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