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

TS source maps look wrong when throwing Error #316

Closed
styfle opened this issue Mar 18, 2019 · 6 comments
Closed

TS source maps look wrong when throwing Error #316

styfle opened this issue Mar 18, 2019 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@styfle
Copy link
Member

styfle commented Mar 18, 2019

I got a report from @javivelasco that the source maps looked wrong for errors in Sentry.

Typically, the .js + .js.map can be used by Sentry to show the original line number that the error was thrown instead of the bundled/webpack line number. However, this was not working so I made a small example to reproduce what I believe is the root cause.

Steps to reproduce

git clone https://github.com/styfle/ncc-bug-sourcemap
cd ncc-bug-sourcemap
yarn

yarn build         # builds with tsc proper
node dist/index.js # note the error output

rm -rf dist

ncc build index.ts --source-map
node dist/index.js # note the error output

The most notable difference is the that some of the lines in the stack trace say .ts instead of .js and don't include a full path.

Diff

image

@styfle styfle added the bug Something isn't working label Mar 18, 2019
@guybedford
Copy link
Contributor

guybedford commented Mar 18, 2019 via email

@styfle
Copy link
Member Author

styfle commented Mar 18, 2019

@guybedford This looks good!

ncc build index.ts --source-map --no-source-map-register
node dist/index.js

Diff

image

@guybedford Why is source map register enabled by default?

@guybedford
Copy link
Contributor

Because ncc stack traces are otherwise completely undecipherable when running in Node.js, and with this register addon we can always know the original file easily from the stack trace without any more work.

@styfle
Copy link
Member Author

styfle commented Mar 18, 2019

Because ncc stack traces are otherwise completely undecipherable when running in Node.js

That makes sense for the use case when the user is not generating source maps (default behavior).

But when the user explicitly requests source maps, it seems strange.

For example, I can't think of a scenario where the user would want to use ncc build index.ts --source-map instead of ncc build index.ts --source-map --no-source-map-register.

Was this option (--no-source-map-register) added to maintain backwards compatibility or is there a use case I'm not thinking of 🤔

@guybedford
Copy link
Contributor

By default, source maps are not generated in ncc build, so that ncc build index.ts will have no source maps in the output.

So ncc build index.ts --source-map && node dist gives you a version with debugging support which is what users might typically want for source maps in a Node.js environment.

The --no-source-map-register is then the opt-out of the Node.js source maps "polyfill".

I'm certainly open to changing things though.

@styfle
Copy link
Member Author

styfle commented Mar 20, 2019

Closing in favor of #319

@styfle styfle closed this as completed Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants