[FEAT] Map stacktraces #773

teoxoy · 2019-06-28T20:47:10Z

closes #117

sourcemap-stacktrace takes in a stacktrace and uses sourcemaps (if it finds any) to map the paths to the sources

teoxoy · 2020-07-03T08:18:32Z

@Conduitry has the issue been fixed in some other PR? Should I close this one?

If not, I can add the code from sourcemap-stacktrace package in sapper and delete the package.
I am not really up to maintaining it anymore, I don't know why I made it a package in the first place...

benmccann · 2020-07-04T03:05:43Z

No, I don't believe this issue has been fixed yet

teoxoy · 2020-07-08T02:58:24Z

So, is there an issue with this PR? Do you guys want the code in sapper?

benmccann · 2020-07-08T13:53:12Z

I think mostly everyone's just been busy. Also, errors weren't showing up at all due to #719, which made this one hard to review. Now that that's been fixed I think it's time to revisit this one.

I can add the code from sourcemap-stacktrace package in sapper and delete the package.
I am not really up to maintaining it anymore, I don't know why I made it a package in the first place...

You said your package was based on node-source-map-support, but exposes the needed functionality. Would you be able to file an issue with node-source-map-support to ask them to expose that functionality? Then we could consider using that package instead of having to add the code directly into Sapper

Also, I would suggest splitting out the linting fixes into a separate PR. We've been working on linting and you can see some of our progress in #1198, but unfortunately that doesn't fix the linting issues you found and we should investigate why. Perhaps we should setup prettier on the codebase

teoxoy · 2020-07-12T04:01:51Z

I rechecked node-source-map-support and it has some node v8 stacktrace specifics + it doesn't look maintained to me.

I can remove the formatting changes and move the src of sourcemap-stacktrace (95 LOC) in the repo.
Let me know if this is how we should move forward with this.

antony · 2020-07-12T07:45:53Z

How did this pass me by? This would be incredibly useful.

I would agree that replicating the source of that module here is probably desirable rather than bringing the whole thing in. Can we include a note so that it's clear that this is the case, and if the source module improves we can incorporate at a later date.

Also as @benmccann says, we don't accept linting fixes in PRs, unless that is the sole purpose of the PR, so they should be removed for this one.

Thanks!

benmccann · 2020-07-13T02:45:10Z

it has some node v8 stacktrace specifics

V8 is the Javascript engine inside of node.js that parses and runs your Javascript. The same V8 engine is used inside of Chrome to run javascript in the Chrome browser. Google open-sourced the V8 engine and the builders of node.js used it to run Javascript in node.js. The current node.js binary cannot work without V8. So I'm not sure why it'd be an issue to have v8 specifics in the code?

If you're talking about Node 8.x instead, I'm also not sure it harms us at all if it supports Node 8.x

it doesn't look maintained to me.

I'm not sure I'd call source-map-support unsupported. It does have a number of open issues and PRs, but also has had some commits regularly and was last updated only two months ago and has 20m weekly downloads.

Anyway, we don't have to block on getting the functionality implemented upstream. I wouldn't want to block this feature if we can't get the support we need. But we should at least ask for it before forking the project and copying the source here that way there's at least a possibility that we don't have to maintain a fork of the project long-term.

teoxoy · 2020-07-13T03:54:19Z

I am aware of the v8/node relationship. What I meant to say is that source-map-support is using the V8 stack trace API which doesn't work for us (this is part of the reason it was removed in #366).

I'm not sure why you refer to source-map-support as upstream since I mention in the readme of sourcemap-stacktrace that it is a reimplementation.

The relevant part to us is 1/6th of source-map-support; sourcemapping a stacktrace is quite simple, it's a matter of going trough each line of the stacktrace and trying to replace the path, line no. and column no.. Doing that by searching in the file that thew the error for a sourceMappingURL and mapping the orignal line and column numbers using source-map.

benmccann · 2020-07-13T04:23:03Z

source-map-support is using the V8 stack trace API which doesn't work for us (this is part of the reason it was removed in #366).

Can you explain why it doesn't work for us? I don't see any reference to it in #366

teoxoy · 2020-07-13T05:31:36Z

notwithstanding that it doesn't currently apply to components

It has been a year since I worked on this - so, I don't actually remember why it didn't work.

benmccann · 2020-07-13T15:11:21Z

Ok. I'm fine with this PR if you're able to move the linting fixes to a new PR and rebase it

teoxoy · 2020-07-16T06:10:53Z

Rebased, removed formatting/linting fixes and moved the src of sourcemap-stacktrace in the repo.

benmccann

thanks! do you think it'd be possible to add a test for the new sourcemapStacktrace functionality?

package.json

teoxoy · 2020-07-22T06:17:45Z

I can't seem to get the test suite running in WSL and I currently don't have the time to debug it and also add tests.

benmccann · 2020-07-22T12:38:45Z

ok. thanks for all your work on this already

pngwn · 2020-07-23T16:23:32Z

This needs tests before it can be merged.

benmccann · 2020-07-24T04:35:25Z

I was just thinking about this some more... Does anyone know why we bundle the server? I wonder if it's necessary. If we didn't bundle it then I don't think we'd need source maps for the stack traces

Update:
I tried not bundling the server. First had to hack Sapper to make server Rollup config optional. Then had to add "type": "module" to the package.json and pass --experimental-modules (flag will not be required in Node 14, but I'm on 12). Then it wasn't finding @sapper/server for some reason and I got stuck. Node requires file extensions on imports, but I tried changing it to @sapper/server.mjs and it still didn't work. It will probably just be easier to write tests for this PR than try to unbundle the server 😄

pngwn · 2020-07-25T00:16:50Z

Bundling, even for the server, generally makes things a bit faster and smaller. It also makes things like building a docker image simpler as the server bundle contains all of the necessary code so you don't need to ship any dependencies.

sdwvit · 2020-08-04T01:03:37Z

please merge & release asap

benmccann · 2020-08-04T02:10:26Z

@sdwvit you can help it get merged. the main thing this needs is some tests. if you'd like to contribute we'd be happy to see this get merged much sooner

habibrosyad · 2020-08-04T13:10:30Z

runtime/src/server/middleware/sourcemap_stacktrace.ts

+                }
+
+                const consumer = new SourceMapConsumer(rawSourceMap);
+                const pos = consumer.originalPositionFor({


Apparently, with the latest version of source-map used here (0.7.3), consumer returns promise, therefore call to originalPositionFor will fail 😕

You are right, it should be version 0.6.1

I'm planing to continue the works that you've done if you don't mind, especially adding tests for this...

Not at all, I mentioned in one of my previous comments that I've been quite busy lately.

benmccann mentioned this pull request May 29, 2020

Prevent client to rerender page over SSR error - fixes #710 #719

Merged

teoxoy changed the title ~~[FEAT] use sourcemap-stacktrace to transform stacktraces~~ [FEAT] Map stacktraces Jul 12, 2020

antony self-assigned this Jul 12, 2020

sourcemap stacktraces

2e5a510

teoxoy force-pushed the sourcemap-stacktrace branch from 5333ab2 to 2e5a510 Compare July 16, 2020 06:04

benmccann reviewed Jul 16, 2020

View reviewed changes

package.json Show resolved Hide resolved

benmccann approved these changes Jul 22, 2020

View reviewed changes

benmccann mentioned this pull request Jul 25, 2020

Stack traces could be better in general #899

Closed

habibrosyad reviewed Aug 4, 2020

View reviewed changes

habibrosyad mentioned this pull request Aug 4, 2020

Support better stack trace with sourcemaps #1357

Merged

4 tasks

benmccann closed this in #1357 Aug 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEAT] Map stacktraces #773

[FEAT] Map stacktraces #773

teoxoy commented Jun 28, 2019 •

edited

Loading

teoxoy commented Jul 3, 2020

benmccann commented Jul 4, 2020

teoxoy commented Jul 8, 2020

benmccann commented Jul 8, 2020

teoxoy commented Jul 12, 2020

antony commented Jul 12, 2020

benmccann commented Jul 13, 2020

teoxoy commented Jul 13, 2020

benmccann commented Jul 13, 2020

teoxoy commented Jul 13, 2020

benmccann commented Jul 13, 2020

teoxoy commented Jul 16, 2020

benmccann left a comment

teoxoy commented Jul 22, 2020

benmccann commented Jul 22, 2020 •

edited

Loading

pngwn commented Jul 23, 2020

benmccann commented Jul 24, 2020 •

edited

Loading

pngwn commented Jul 25, 2020

sdwvit commented Aug 4, 2020

benmccann commented Aug 4, 2020

habibrosyad Aug 4, 2020 •

edited

Loading

teoxoy Aug 4, 2020

habibrosyad Aug 4, 2020

teoxoy Aug 4, 2020

[FEAT] Map stacktraces #773

[FEAT] Map stacktraces #773

Conversation

teoxoy commented Jun 28, 2019 • edited Loading

teoxoy commented Jul 3, 2020

benmccann commented Jul 4, 2020

teoxoy commented Jul 8, 2020

benmccann commented Jul 8, 2020

teoxoy commented Jul 12, 2020

antony commented Jul 12, 2020

benmccann commented Jul 13, 2020

teoxoy commented Jul 13, 2020

benmccann commented Jul 13, 2020

teoxoy commented Jul 13, 2020

benmccann commented Jul 13, 2020

teoxoy commented Jul 16, 2020

benmccann left a comment

Choose a reason for hiding this comment

teoxoy commented Jul 22, 2020

benmccann commented Jul 22, 2020 • edited Loading

pngwn commented Jul 23, 2020

benmccann commented Jul 24, 2020 • edited Loading

pngwn commented Jul 25, 2020

sdwvit commented Aug 4, 2020

benmccann commented Aug 4, 2020

habibrosyad Aug 4, 2020 • edited Loading

Choose a reason for hiding this comment

teoxoy Aug 4, 2020

Choose a reason for hiding this comment

habibrosyad Aug 4, 2020

Choose a reason for hiding this comment

teoxoy Aug 4, 2020

Choose a reason for hiding this comment

teoxoy commented Jun 28, 2019 •

edited

Loading

benmccann commented Jul 22, 2020 •

edited

Loading

benmccann commented Jul 24, 2020 •

edited

Loading

habibrosyad Aug 4, 2020 •

edited

Loading