fix: improve source code location metadata accuracy #8362

dominikg · 2023-03-08T16:25:19Z

by accounting for preprocessor sourcemap and one based counting

fixes #8360

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

… preprocessor sourcemap and one based counting

dominikg · 2023-03-08T16:25:58Z

src/compiler/compile/Component.ts

+			if (tracer) {
+				location = originalPositionFor(tracer, location);
+			}
+			location.line += 1; // make line one based


is this really just zero based vs one based or another cause?

I'm wondering why this all still works the same, since you're always adding +1 to the line, regardless of a source map existing or not.

you're already adding 1 above: https://github.com/sveltejs/svelte/pull/8362/files#r1135771594

so I think this is really adding 2 which seems wrong to me

in a locally linked vite project with sveltekit-ts it produces correct positions in __svelte_meta.

I agree that this looks off and so far havn't been able to understand why it produces the correct result.

src/compiler/compile/Component.ts

dominikg · 2023-03-08T16:27:27Z

test/runtime/samples/element-source-location/_config.js

@@ -11,14 +11,14 @@ export default {

 		assert.deepEqual(h1.__svelte_meta.loc, {
 			file: path.relative(process.cwd(), path.resolve(__dirname, 'main.svelte')),
-			line: 4,
+			line: 5,


looking at main.svelte, the h1 sits at line 5, not line 4

I've reverted these changes since it seems like there's some other bug that was present previously. Perhaps something like Rich-Harris/magic-string#248 which was causing some of our other source map tests to assert unexpected behavior

dominikg · 2023-03-08T16:29:43Z

test/runtime/samples/element-source-location/_config.js

 			column: 0,
 			char: 53
 		});

 		assert.deepEqual(p.__svelte_meta.loc, {
 			file: path.relative(process.cwd(), path.resolve(__dirname, 'Foo.svelte')),
-			line: 1,
+			line: 2,


same for the p, the previous combination of line 1, col 1 and char 7 would require cramming 6 chars at col 0.

vercel · 2023-03-08T16:32:17Z

@dominikg is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

dominikg · 2023-03-09T12:12:53Z

The type error happening in CI is caused by trace-mapping not having a valid "types" reference in package.json and the typescript version svelte uses doesn't understand exports map yet.

src/compiler/compile/Component.ts:1:47 - error TS2307: Cannot find module '@jridgewell/trace-mapping' or its corresponding type declarations.

1 import { TraceMap, originalPositionFor } from '@jridgewell/trace-mapping';

given this and the fact that changing line output to be one based could be considered a breaking change i'd suggest to only add this with svelte 4, where we can update typescript and make it a breaking change, even if only relevant for some tooling during dev.

dummdidumm · 2023-03-09T12:27:08Z

That's strange, that packages does have type definitions both through top level typings as well as in exports through types. Is this the Rollup plugin somehow messing up?

dominikg · 2023-03-09T12:41:08Z

see my PR i sent them, it should be "types" in top level package.json, not "typings"

src/compiler/compile/Component.ts

Co-authored-by: Ben McCann <[email protected]>

dominikg · 2023-03-14T19:35:23Z

looks like typescript doesn't ignore missing types with @ts-expect-error, tried @ts-ignore locally too.
If we wanted to merge this in svelte3, i guess we'd have to add fake type locally.

src/compiler/compile/Component.ts

fixes sveltejs#8359

Instead of "both", which doesn't make sense at that point.

Co-authored-by: Ben McCann <[email protected]>

- upgrade to TypeScript 5 - upgrade @ampproject/remapping - remove obsolete workarounds --------- Co-authored-by: Simon Holthausen <[email protected]>

bump to rollup 3. Includes reworking the "treat those imports as external" a bit so that Rollup builds correctly but doesn't bundle some of the (now relative) imports --------- Co-authored-by: Simon Holthausen <[email protected]>

benmccann · 2023-04-12T18:16:22Z

I've gotten this down to a single failing test:

  1) sourcemaps
       sourcemap-basename:

      AssertionError [ERR_ASSERTION]: js.map.sources is wrong
      + expected - actual

       [
      -  "external_code.css"
      +  "input.svelte"
       ]

I'm not sure why that's failing, but it does seem wrong to me that it's returning a .css file for js.map.sources

naturalethic · 2023-04-19T22:51:56Z

This should also properly calculate the character position ({ loc: { char } }). In my testing, it is not.

dummdidumm

The test failure comes from some sources not showing up when they should, I think this has to do with how remapping handles mapping. I'm not sure if this is related to this issue or if we're doing something wrong or if the tests need adjustment.

Edit: After more digging I'm somewhat sure this may be a bug in our implementation. It happens when combining two source maps. The first one is the JS map which points to the original Svelte positions already, the second one is from the Svelte map after preprocessing pointing to the original Svelte positions. Now remapping traces from the JS map the what's supposed to be original Svelte position to a CSS position because that's what it finds in the Svelte map at that position. So either the JS map needs to skip the preprocess map, or the JS map needs to point to the preprocessed Svelte version.

Edit 2: Ok I think I found the problem - due to this code change, locator now directly points to the original Svelte version, which is good for the meta thing. But it's bad for the source map combination since that's now unnecessary. So one has to change.

dummdidumm · 2023-06-21T12:08:09Z

test/runtime/samples/element-source-location/_config.js

 			line: 4,
 			column: 0,
 			char: 53
 		});

 		assert.deepEqual(p.__svelte_meta.loc, {
 			file: path.relative(process.cwd(), path.resolve(__dirname, 'Foo.svelte')),
+			// TODO: fix this. the p is on line 2


I think these tests are correct, as source maps are zero line based. There's actually code in Element/index.js where it substracts one from the returned location to account for that. Why do you think these are incorrect?

source maps are 0-based, but locator is 1-based because it gets printed out to the user and users would expect it to be 1-based:

svelte/src/compiler/compile/Component.ts

Line 524 in 9aca05b

`${warning.message} (${start.line}:${start.column})\n${frame}`

I didn't realize that this loc is different than what's in locator because I missed the -1 here:

svelte/packages/svelte/src/compiler/compile/render_dom/wrappers/Element/index.js

Line 590 in b8918fa

b`@add_location(${this.var}, ${renderer.file_var}, ${loc.line - 1}, ${loc.column}, ${

it's pretty confusing to me to keep track of where things are 0-based and where they're 1-based. I wonder if we can use this opportunity to make it 0-based everywhere and just make it 1-based at the very last second when it's printed to a user

dummdidumm · 2023-06-21T12:10:00Z

src/compiler/compile/Component.ts

+				location = originalPositionFor(tracer, location);
+				// originalPositionFor returns 0-based lines?
+				// https://github.com/jridgewell/trace-mapping/issues/27
+				location.line += 1;


judging from some other code in language tools and the fact that tracemapping says it is a drop-in replacement for source-map which has line-1-based return values, this should not be done

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

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. Sadly we can't map the character offset because for that we would need to the original source contents which we don't have in this context. fixes #8360 closes #8362

fix: improve source code location metadata accuracy by accounting for…

11fc31e

… preprocessor sourcemap and one based counting

dominikg commented Mar 8, 2023

View reviewed changes

src/compiler/compile/Component.ts Show resolved Hide resolved

dominikg commented Mar 8, 2023

View reviewed changes

dominikg mentioned this pull request Mar 9, 2023

fix: add "types" field to package.json jridgewell/trace-mapping#24

Merged

dummdidumm reviewed Mar 14, 2023

View reviewed changes

src/compiler/compile/Component.ts Outdated Show resolved Hide resolved

Update src/compiler/compile/Component.ts

f42aa9c

benmccann reviewed Mar 14, 2023

View reviewed changes

src/compiler/compile/Component.ts Outdated Show resolved Hide resolved

benmccann reviewed Mar 14, 2023

View reviewed changes

src/compiler/compile/Component.ts Outdated Show resolved Hide resolved

dominikg and others added 2 commits March 14, 2023 20:27

fix: add ts-expect-error for invalid trace-mapping type

d89022f

Co-authored-by: Ben McCann <[email protected]>

fix: format

22eaaf2

Co-authored-by: Ben McCann <[email protected]>

benmccann reviewed Mar 14, 2023

View reviewed changes

src/compiler/compile/Component.ts Show resolved Hide resolved

dummdidumm added this to the 4.x milestone Apr 3, 2023

Merge branch 'master' into fix/svelte-meta-pos

2dea46b

benmccann reviewed Apr 7, 2023

View reviewed changes

src/compiler/compile/Component.ts Outdated Show resolved Hide resolved

xxkl1 and others added 4 commits April 11, 2023 15:20

fix: html space entities lost in component slot (sveltejs#8464)

765023d

fixes sveltejs#8359

breaking: send in/out to transition fn (sveltejs#8318)

75aec41

Instead of "both", which doesn't make sense at that point.

chore: remove node<14 tests (sveltejs#8482)

c729829

Co-authored-by: Ben McCann <[email protected]>

TypeScript 4

61312c8

benmccann changed the base branch from master to version-4 April 11, 2023 20:50

benmccann added 4 commits April 11, 2023 13:52

merge version-4 branch

7d3b368

add a new @ts-ignore

4547d3f

lockfile update

e8c1db9

chore: simplify Svelte 4 CI (sveltejs#8487)

eedacc9

benmccann and others added 10 commits April 12, 2023 09:01

chore: bump engines field (sveltejs#8489)

abd760d

chore: upgrade to TypeScript 5 (sveltejs#8488)

1e2cfa4

- upgrade to TypeScript 5 - upgrade @ampproject/remapping - remove obsolete workarounds --------- Co-authored-by: Simon Holthausen <[email protected]>

chore: Svelte 4 dependency upgrades (sveltejs#8486)

42e0f7d

chore: upgrade rollup (sveltejs#8491)

c9ccd6e

bump to rollup 3. Includes reworking the "treat those imports as external" a bit so that Rollup builds correctly but doesn't bundle some of the (now relative) imports --------- Co-authored-by: Simon Holthausen <[email protected]>

merge version-4 branch

8c9198d

package-lock cleanup

16ab649

update trace-mapping library

1533c9f

Update src/compiler/compile/Component.ts

7410321

fix most tests

56146d0

revert unrelated changes and add TODOs

eab99a4

dummdidumm force-pushed the version-4 branch from 350c6c3 to 236ffa8 Compare April 18, 2023 09:12

merge version-4 branch

2c92c2f

fix changelog

9aca05b

benmccann removed this from the 4.x milestone Jun 19, 2023

benmccann deleted the branch sveltejs:master June 20, 2023 20:45

benmccann closed this Jun 20, 2023

benmccann reopened this Jun 20, 2023

benmccann changed the base branch from version-4 to master June 20, 2023 20:54

dummdidumm reviewed Jun 21, 2023

View reviewed changes

dummdidumm mentioned this pull request Jun 21, 2023

fix: account for sourcemap in meta info #8778

Merged

8 tasks

dummdidumm closed this in #8778 Jun 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improve source code location metadata accuracy #8362

fix: improve source code location metadata accuracy #8362

dominikg commented Mar 8, 2023

dominikg Mar 8, 2023

dummdidumm Mar 14, 2023

benmccann Mar 14, 2023

dominikg Mar 14, 2023

dominikg Mar 8, 2023

benmccann Apr 12, 2023

dominikg Mar 8, 2023 •

edited

Loading

vercel bot commented Mar 8, 2023

dominikg commented Mar 9, 2023 •

edited

Loading

dummdidumm commented Mar 9, 2023

dominikg commented Mar 9, 2023

dominikg commented Mar 14, 2023

benmccann commented Apr 12, 2023

naturalethic commented Apr 19, 2023

dummdidumm left a comment •

edited

Loading

dummdidumm Jun 21, 2023

benmccann Jun 21, 2023

dummdidumm Jun 21, 2023

fix: improve source code location metadata accuracy #8362

fix: improve source code location metadata accuracy #8362

Conversation

dominikg commented Mar 8, 2023

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

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikg Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

vercel bot commented Mar 8, 2023

dominikg commented Mar 9, 2023 • edited Loading

dummdidumm commented Mar 9, 2023

dominikg commented Mar 9, 2023

dominikg commented Mar 14, 2023

benmccann commented Apr 12, 2023

naturalethic commented Apr 19, 2023

dummdidumm left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikg Mar 8, 2023 •

edited

Loading

dominikg commented Mar 9, 2023 •

edited

Loading

dummdidumm left a comment •

edited

Loading