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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c917eb1
fix: account for sourcemap in meta info
dummdidumm Jun 21, 2023
903ba2e
changeset
dummdidumm Jun 21, 2023
10aadf4
Apply suggestions from code review
dummdidumm Jun 22, 2023
61ff4ea
doc here, too
dummdidumm Jun 22, 2023
a6ddb2d
lazy init for a tiny speed boost on prod builds
dummdidumm Jun 22, 2023
f4108c0
add comment about limitation
dummdidumm Jun 22, 2023
db686cf
test
dummdidumm Jun 22, 2023
a03ec02
package.json spaces
dummdidumm Jun 22, 2023
f0ff443
fix comment
dummdidumm Jun 22, 2023
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
View file
Open in desktop
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
269 changes: 135 additions & 134 deletions packages/svelte/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,135 +1,136 @@
{
"name": "svelte",
"version": "4.0.0-next.2",
"description": "Cybernetically enhanced web apps",
"type": "module",
"module": "src/runtime/index.js",
"main": "src/runtime/index.js",
"files": [
"src",
"types",
"compiler.*",
"register.js",
"index.d.ts",
"store.d.ts",
"animate.d.ts",
"transition.d.ts",
"easing.d.ts",
"motion.d.ts",
"action.d.ts",
"elements.d.ts",
"README.md"
],
"exports": {
"./package.json": "./package.json",
".": {
"types": "./types/index.d.ts",
"browser": {
"import": "./src/runtime/index.js"
},
"import": "./src/runtime/ssr.js"
},
"./compiler": {
"types": "./types/index.d.ts",
"import": "./src/compiler/index.js",
"require": "./compiler.cjs"
},
"./action": {
"types": "./types/index.d.ts"
},
"./animate": {
"types": "./types/index.d.ts",
"import": "./src/runtime/animate/index.js"
},
"./easing": {
"types": "./types/index.d.ts",
"import": "./src/runtime/easing/index.js"
},
"./internal": {
"import": "./src/runtime/internal/index.js"
},
"./motion": {
"types": "./types/index.d.ts",
"import": "./src/runtime/motion/index.js"
},
"./store": {
"types": "./types/index.d.ts",
"import": "./src/runtime/store/index.js"
},
"./transition": {
"types": "./types/index.d.ts",
"import": "./src/runtime/transition/index.js"
},
"./elements": {
"types": "./elements.d.ts"
}
},
"engines": {
"node": ">=16"
},
"types": "types/index.d.ts",
"scripts": {
"format": "prettier . --cache --plugin-search-dir=. --write",
"check": "prettier . --cache --plugin-search-dir=. --check",
"test": "vitest run && echo \"manually check that there are no type errors in test/types by opening the files in there\"",
"build": "rollup -c && pnpm types",
"generate:version": "node ./scripts/generate-version.js",
"dev": "rollup -cw",
"posttest": "agadoo src/internal/index.js",
"prepublishOnly": "pnpm build",
"types": "node ./scripts/generate-dts.js",
"lint": "eslint \"{src,test}/**/*.{ts,js}\" --cache"
},
"repository": {
"type": "git",
"url": "https://github.com/sveltejs/svelte.git",
"directory": "packages/svelte"
},
"keywords": [
"UI",
"framework",
"templates",
"templating"
],
"author": "Rich Harris",
"license": "MIT",
"bugs": {
"url": "https://github.com/sveltejs/svelte/issues"
},
"homepage": "https://svelte.dev",
"dependencies": {
"@ampproject/remapping": "^2.2.1",
"@jridgewell/sourcemap-codec": "^1.4.15",
"acorn": "^8.8.2",
"aria-query": "^5.2.1",
"axobject-query": "^3.2.1",
"code-red": "^1.0.3",
"css-tree": "^2.3.1",
"estree-walker": "^3.0.3",
"is-reference": "^3.0.1",
"locate-character": "^3.0.0",
"magic-string": "^0.30.0",
"periscopic": "^3.1.0"
},
"devDependencies": {
"@playwright/test": "^1.34.3",
"@rollup/plugin-commonjs": "^24.1.0",
"@rollup/plugin-json": "^6.0.0",
"@rollup/plugin-node-resolve": "^15.0.2",
"@sveltejs/eslint-config": "^6.0.4",
"@types/aria-query": "^5.0.1",
"@types/estree": "^1.0.1",
"@types/node": "^14.14.31",
"agadoo": "^3.0.0",
"dts-buddy": "^0.1.7",
"esbuild": "^0.17.19",
"happy-dom": "^9.18.3",
"jsdom": "^21.1.1",
"kleur": "^4.1.5",
"rollup": "^3.20.2",
"source-map": "^0.7.4",
"tiny-glob": "^0.2.9",
"typescript": "^5.0.4",
"vitest": "^0.31.1"
}
}
"name": "svelte",
"version": "4.0.0-next.2",
"description": "Cybernetically enhanced web apps",
"type": "module",
"module": "src/runtime/index.js",
"main": "src/runtime/index.js",
"files": [
"src",
"types",
"compiler.*",
"register.js",
"index.d.ts",
"store.d.ts",
"animate.d.ts",
"transition.d.ts",
"easing.d.ts",
"motion.d.ts",
"action.d.ts",
"elements.d.ts",
"README.md"
],
"exports": {
"./package.json": "./package.json",
".": {
"types": "./types/index.d.ts",
"browser": {
"import": "./src/runtime/index.js"
},
"import": "./src/runtime/ssr.js"
},
"./compiler": {
"types": "./types/index.d.ts",
"import": "./src/compiler/index.js",
"require": "./compiler.cjs"
},
"./action": {
"types": "./types/index.d.ts"
},
"./animate": {
"types": "./types/index.d.ts",
"import": "./src/runtime/animate/index.js"
},
"./easing": {
"types": "./types/index.d.ts",
"import": "./src/runtime/easing/index.js"
},
"./internal": {
"import": "./src/runtime/internal/index.js"
},
"./motion": {
"types": "./types/index.d.ts",
"import": "./src/runtime/motion/index.js"
},
"./store": {
"types": "./types/index.d.ts",
"import": "./src/runtime/store/index.js"
},
"./transition": {
"types": "./types/index.d.ts",
"import": "./src/runtime/transition/index.js"
},
"./elements": {
"types": "./elements.d.ts"
}
},
"engines": {
"node": ">=16"
},
"types": "types/index.d.ts",
"scripts": {
"format": "prettier . --cache --plugin-search-dir=. --write",
"check": "prettier . --cache --plugin-search-dir=. --check",
"test": "vitest run && echo \"manually check that there are no type errors in test/types by opening the files in there\"",
"build": "rollup -c && pnpm types",
"generate:version": "node ./scripts/generate-version.js",
"dev": "rollup -cw",
"posttest": "agadoo src/internal/index.js",
"prepublishOnly": "pnpm build",
"types": "node ./scripts/generate-dts.js",
"lint": "eslint \"{src,test}/**/*.{ts,js}\" --cache"
},
"repository": {
"type": "git",
"url": "https://github.com/sveltejs/svelte.git",
"directory": "packages/svelte"
},
"keywords": [
"UI",
"framework",
"templates",
"templating"
],
"author": "Rich Harris",
"license": "MIT",
"bugs": {
"url": "https://github.com/sveltejs/svelte/issues"
},
"homepage": "https://svelte.dev",
"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",
"code-red": "^1.0.3",
"css-tree": "^2.3.1",
"estree-walker": "^3.0.3",
"is-reference": "^3.0.1",
"locate-character": "^3.0.0",
"magic-string": "^0.30.0",
"periscopic": "^3.1.0"
},
"devDependencies": {
"@playwright/test": "^1.34.3",
"@rollup/plugin-commonjs": "^24.1.0",
"@rollup/plugin-json": "^6.0.0",
"@rollup/plugin-node-resolve": "^15.0.2",
"@sveltejs/eslint-config": "^6.0.4",
"@types/aria-query": "^5.0.1",
"@types/estree": "^1.0.1",
"@types/node": "^14.14.31",
"agadoo": "^3.0.0",
"dts-buddy": "^0.1.7",
"esbuild": "^0.17.19",
"happy-dom": "^9.18.3",
"jsdom": "^21.1.1",
"kleur": "^4.1.5",
"rollup": "^3.20.2",
"source-map": "^0.7.4",
"tiny-glob": "^0.2.9",
"typescript": "^5.0.4",
"vitest": "^0.31.1"
}
}
26 changes: 25 additions & 1 deletion packages/svelte/src/compiler/compile/Component.js
View file
Open in desktop
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,18 @@ export default class Component {
/** @type {string} */
file;

/** @type {(c: number) => { line: number; column: number }} */
/**
* Use this for source mappings. Use `meta_locate` for the meta data on the dom elements.
dummdidumm marked this conversation as resolved.
Show resolved Hide resolved
* @type {(c: number) => { line: number; column: number }}
*/
locate;

/**
* Use this for the meta data on the dom elements. Use `locate` for source mappings.
dummdidumm marked this conversation as resolved.
Show resolved Hide resolved
* @type {(c: number) => { line: number; column: number }}
*/
meta_locate;

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

Expand Down Expand Up @@ -199,7 +209,21 @@ 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 });
// @ts-expect-error - fix the type of CompileOptions.sourcemap
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)

if (tracer) {
// originalPositionFor returns 1-based lines like locator
location = originalPositionFor(tracer, location);
}
return location;
};

// styles
this.stylesheet = new Stylesheet({
source,
Expand Down
4 changes: 4 additions & 0 deletions packages/svelte/src/compiler/compile/render_dom/Renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ export default class Renderer {
/** @type {(c: number) => { line: number; column: number }} */
locate;

/** @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 +76,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
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/compile/render_dom/wrappers/Element/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,7 +585,7 @@ 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}, ${
this.node.start
Expand Down
3 changes: 3 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

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