-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Sourcemapped stack traces #266
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
Conversation
| } | ||
|
|
||
| try { | ||
| const { contents } = await snowpack.loadUrl(address, { isSSR: true, encoding: 'utf8' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought sourcemap file URLs were paths from the project root on the file system. It might be helpful to add a comment explaining when this would occur. I'm also wondering about the fact that this is needed here and not in page.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the Resolving sources section of the standard,
If the sources are not absolute URLs after prepending of the “sourceRoot”, the sources are resolved relative to the SourceMap (like resolving script src in a html document).
In other words, to find the original file for a give segment, find the index into the sources array, and do path.resolve(sourcemap_file, source). There's some ambiguity here because information gets lost between the filesystem and a webserver, but this is the most reliable way to reconstruct a filepath.
I'm also wondering about the fact that this is needed here and not in
page.js
loadUrl is invoked for things other than rendering pages (e.g. setup, endpoints)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we need to call loadUrl for endpoints and not pages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's called for both
packages/kit/src/renderer/page.js
Outdated
| @@ -1,5 +1,5 @@ | |||
| import devalue from 'devalue'; | |||
| import { createReadStream, existsSync } from 'fs'; | |||
| import { createReadStream, existsSync, fstat, readFileSync } from 'fs'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fstat is unused
|
Decided to remove the sourcemapping from prod, for now — just trades one set of headaches for another. Could be revisited in future. |
This implements sourcemapped stack traces for SSR errors in dev and prod. We might argue that we don't want sourcemaps in prod, which could certainly make life easier since source-map is a cumbersome dependency (as it's not pure JS — not sure if it will even work everywhere we want to deploy to).
Also, it needs
fsso presumably wouldn't work in a cloudflare worker, for example.Also also, the filenames are wrong because of this bug: FredKSchott/snowpack#1941