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: account for sourcemap in meta info #8778

Merged
merged 9 commits into from Jun 22, 2023
Merged

fix: account for sourcemap in meta info #8778

merged 9 commits into from Jun 22, 2023

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Jun 21, 2023

We need to use a different method for getting the meta info because locate is used to help construct the source map that references the preprocessed Svelte file. If we would now add source maps to that locate function it would go the the original source directly which means skipping potentially intermediate source maps which we would need in other situations.

Also adds a new dependency, cc @benmccann that might affect the blog post

fixes #8360
closes #8362

TODO:

  • think one more time if there really isn't a way to reuse the other stuff (didn't find a better way)
  • needs to also convert the character (update: limitation we can't solve, noted in a TODO)
  • tests

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

  • Run the tests with pnpm test and lint the project with pnpm lint

We need to use a different method for getting the meta info because `locate` is used to help construct the source map that references the preprocessed Svelte file. If we would now add source maps to that `locate` function it would go the the original source directly which means skipping potentially intermediate source maps which we would need in other situations.

fixes #8360
closes #8362
@dummdidumm dummdidumm marked this pull request as draft June 21, 2023 15:07
packages/svelte/src/compiler/compile/Component.js Outdated Show resolved Hide resolved
packages/svelte/src/compiler/compile/Component.js Outdated Show resolved Hide resolved
const tracer = compile_options.sourcemap ? new TraceMap(compile_options.sourcemap) : undefined;
this.meta_locate = (c) => {
/** @type {{ line: number, column: number }} */
let location = this.locate(c);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's confusing to me that we used a 1-based locator for meta_locate and then subtract 1. if we just used the default sourcemap offset we wouldn't have to deal with that

we could make meta_locate use a separate 0-based locator. would that be more expensive though?

or we could make locate be 0-based and then just add 1 when we're printing it out. I think that'd be clearer because sourcemaps are 0-based and then we can just treat everything in the codebase as being 0-based. is locate exposed externally though and would that be a breaking change that results in a more difficult API for people needing to print stack traces?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't change much as the source mapping function works on 1-based lines so we have back and forth either way. I think the way it's now is the most consistent (both locate and meta_locate are 1-based) and you do whatever you need to do on the consumer side (and 1-based is needed more often)

@benmccann
Copy link
Member

Also adds a new dependency, cc @benmccann that might affect the blog post

It looks like @jridgewell/trace-mapping was already a transitive dependency, so this doesn't change much

@dummdidumm
Copy link
Member Author

Ah nice 👍

@dominikg
Copy link
Member

Nice, I think this solves my concern about internal state in the locator as well. I don't really like having both 0 and 1 index things going on but it is what it is.

@benmccann
Copy link
Member

Nice, I think this solves my concern about internal state in the locator as well. I don't really like having both 0 and 1 index things going on but it is what it is.

Also, locate-character has been updated to the latest version where I think that issue was fixed

@benmccann benmccann added this to the 4.x milestone Jun 21, 2023
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@dummdidumm dummdidumm marked this pull request as ready for review June 22, 2023 09:55
@dummdidumm dummdidumm merged commit ef1b98f into master Jun 22, 2023
6 of 7 checks passed
@dummdidumm dummdidumm deleted the meta-locate branch June 22, 2023 09:59
@github-actions github-actions bot mentioned this pull request Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants