Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Support better stack trace with sourcemaps #1357

Merged
merged 4 commits into from
Aug 10, 2020

Conversation

habibrosyad
Copy link
Contributor

This PR is a continuity of #773 by adding test and fix found issues.

Closes #117
Closes #773

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

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests tests with npm test or yarn test)

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks so much for picking this PR up!

runtime/src/server/middleware/sourcemap_stacktrace.ts Outdated Show resolved Hide resolved
}
);

file_cache.clear();
Copy link
Member

@benmccann benmccann Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure what the point of the file_cache is if it's cleared everytime we try to source map an exception

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that in a stack trace, sometimes you can hit the same file with different lines and cols. The file_cache there is just a means to avoid rereading that file multiple times in a single call to sourcemap_stacktrace.

runtime/src/server/middleware/sourcemap_stacktrace.ts Outdated Show resolved Hide resolved
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me

@benmccann
Copy link
Member

@habibrosyad this PR will need to be rebased

@habibrosyad
Copy link
Contributor Author

@benmccann done...

Copy link
Member

@antony antony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is brilliant. Makes diagnosing server-errors easy. Thank you!

@benmccann benmccann merged commit 266828d into sveltejs:master Aug 10, 2020
@habibrosyad habibrosyad deleted the feat-773 branch August 11, 2020 03:37
trmcnvn pushed a commit to metafy-gg/sapper that referenced this pull request Aug 15, 2020
Co-authored-by: Teoxoy <28601907+Teoxoy@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emit and Consume Sourcemaps
4 participants