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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/silly-ladybugs-marry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: account for preprocessor source maps when calculating meta info
2 changes: 1 addition & 1 deletion .prettierrc
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
}
},
{
"files": ["README.md", "packages/*/README.md"],
"files": ["README.md", "packages/*/README.md", "**/package.json"],
"options": {
"useTabs": false,
"tabWidth": 2
Expand Down
3 changes: 2 additions & 1 deletion packages/svelte/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
"dependencies": {
"@ampproject/remapping": "^2.2.1",
"@jridgewell/sourcemap-codec": "^1.4.15",
"@jridgewell/trace-mapping": "^0.3.18",
"acorn": "^8.8.2",
"aria-query": "^5.2.1",
"axobject-query": "^3.2.1",
Expand Down Expand Up @@ -132,4 +133,4 @@
"typescript": "^5.0.4",
"vitest": "^0.31.1"
}
}
}
32 changes: 31 additions & 1 deletion packages/svelte/src/compiler/compile/Component.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { TraceMap, originalPositionFor } from '@jridgewell/trace-mapping';
import { walk } from 'estree-walker';
import { getLocator } from 'locate-character';
import { reserved, is_valid } from '../utils/names.js';
Expand Down Expand Up @@ -142,9 +143,20 @@ export default class Component {
/** @type {string} */
file;

/** @type {(c: number) => { line: number; column: number }} */
/**
* Use this for stack traces. It is 1-based and acts on pre-processed sources.
* Use `meta_locate` for metadata on DOM elements.
* @type {(c: number) => { line: number; column: number }}
*/
locate;

/**
* Use this for metadata on DOM elements. It is 1-based and acts on sources that have not been pre-processed.
* Use `locate` for source mappings.
* @type {(c: number) => { line: number; column: number }}
*/
meta_locate;

/** @type {import('./nodes/Element.js').default[]} */
elements = [];

Expand Down Expand Up @@ -199,7 +211,25 @@ export default class Component {
.replace(process.cwd(), '')
.replace(regex_leading_directory_separator, '')
: compile_options.filename);

// line numbers in stack trace frames are 1-based. source maps are 0-based
this.locate = getLocator(this.source, { offsetLine: 1 });
/** @type {TraceMap | null | undefined} initialise lazy because only used in dev mode */
let tracer;
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)

if (tracer === undefined) {
// @ts-expect-error - fix the type of CompileOptions.sourcemap
tracer = compile_options.sourcemap ? new TraceMap(compile_options.sourcemap) : null;
}
if (tracer) {
// originalPositionFor returns 1-based lines like locator
location = originalPositionFor(tracer, location);
}
return location;
};

// styles
this.stylesheet = new Stylesheet({
source,
Expand Down
14 changes: 13 additions & 1 deletion packages/svelte/src/compiler/compile/render_dom/Renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,20 @@ export default class Renderer {
/** @type {import('estree').Identifier} */
file_var;

/** @type {(c: number) => { line: number; column: number }} */
/**
* Use this for stack traces. It is 1-based and acts on pre-processed sources.
* Use `meta_locate` for metadata on DOM elements.
* @type {(c: number) => { line: number; column: number }}
*/
locate;

/**
* Use this for metadata on DOM elements. It is 1-based and acts on sources that have not been pre-processed.
* Use `locate` for source mappings.
* @type {(c: number) => { line: number; column: number }}
*/
meta_locate;

/**
* @param {import('../Component.js').default} component
* @param {import('../../interfaces.js').CompileOptions} options
Expand All @@ -73,6 +84,7 @@ export default class Renderer {
this.component = component;
this.options = options;
this.locate = component.locate; // TODO messy
this.meta_locate = component.meta_locate; // TODO messy
this.file_var = options.dev && this.component.get_unique_name('file');
component.vars
.filter((v) => !v.hoistable || (v.export_name && !v.module))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -585,9 +585,11 @@ export default class ElementWrapper extends Wrapper {
);
}
if (renderer.options.dev) {
const loc = renderer.locate(this.node.start);
const loc = renderer.meta_locate(this.node.start);
block.chunks.hydrate.push(
b`@add_location(${this.var}, ${renderer.file_var}, ${loc.line - 1}, ${loc.column}, ${
// TODO this.node.start isn't correct if there's a source map. But since we don't know how the
// original source file looked, there's not much we can do.
this.node.start
});`
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import MagicString from 'magic-string';
import * as path from 'node:path';

// fake preprocessor by doing transforms on the source
const str = new MagicString(
`<script>
type Foo = 'foo';
let foo = 'foo';
</script>

<h1>{foo}</h1>
`.replace(/\r\n/g, '\n')
);
str.remove(8, 26); // remove line type Foo = ...
str.remove(55, 56); // remove whitespace before <h1>

export default {
compileOptions: {
dev: true,
sourcemap: str.generateMap({ hires: true })
},

test({ assert, target }) {
const h1 = target.querySelector('h1');

assert.deepEqual(h1.__svelte_meta.loc, {
file: path.relative(process.cwd(), path.resolve(__dirname, 'main.svelte')),
line: 5, // line 4 in main.svelte, but that's the preprocessed code, the original code is above in the fake preprocessor
column: 1, // line 0 in main.svelte, but that's the preprocessed code, the original code is above in the fake preprocessor
char: 38 // TODO this is wrong but we can't backtrace it due to limitations, see add_location function usage comment for more info
});
}
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<script>
let foo = 'foo';
</script>

<h1>{foo}</h1>
3 changes: 3 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.