Skip to content

Fix sourcemap chaining#355

Merged
styfle merged 7 commits intomasterfrom
sourcemap
Apr 19, 2019
Merged

Fix sourcemap chaining#355
styfle merged 7 commits intomasterfrom
sourcemap

Conversation

@guybedford
Copy link
Contributor

@guybedford guybedford commented Apr 18, 2019

This fixes the loaders to support sourcemap chaining properly, and includes a simple integration test to show the TypeScript sourcemaps are that the right offset.

Fixes #353

There will likely be more work to be done here in due course, which we can improve over time (in terms of ensuring the fidelity of the source maps, not their accuracy).

Prerequisite on vercel/webpack-asset-relocator-loader#21.

@guybedford guybedford requested a review from styfle as a code owner April 18, 2019 20:45
@guybedford
Copy link
Contributor Author

We should also keep an eye on performance with this change.

@codecov-io
Copy link

codecov-io commented Apr 18, 2019

Codecov Report

Merging #355 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #355   +/-   ##
=======================================
  Coverage   74.61%   74.61%           
=======================================
  Files          12       12           
  Lines         386      386           
=======================================
  Hits          288      288           
  Misses         98       98
Impacted Files Coverage Δ
src/index.js 84.47% <ø> (ø) ⬆️
src/loaders/uncacheable.js 100% <100%> (ø) ⬆️
src/loaders/empty-loader.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30931b9...8f52204. Read the comment docs.

{
args: ["run", "test/fixtures/fail.ts"],
expect (code, stdout, stderr) {
return code === 1 && stderr.toString().indexOf('fail.ts:2:1') !== -1;
Copy link
Member

Choose a reason for hiding this comment

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

So the column number is still wrong (1), even after changing devtool?

@styfle
Copy link
Member

styfle commented Apr 19, 2019

@guybedford I just checked out this branch and built the code to see how it would work with #353 and it is actually worse.

git clone https://github.com/styfle/ncc-bug-stack
cd ncc-bug-stack
git checkout ts
yarn
rm -rf dist && ../ncc/dist/ncc/cli.js build index.ts --source-map
node dist

Visit http://localhost:3000

Expected: ncc-bug-stack/index.ts:4

Actual: ncc-bug-stack/dist/index.js:61

@guybedford
Copy link
Contributor Author

@styfle interesting, must have changed since my first tests. I've pushed up a patch with a fix.

@styfle
Copy link
Member

styfle commented Apr 19, 2019

First I ran this in the ncc directory

git pull && rm -rf node_modules && yarn && yarn build

Then I ran this in the ncc-bug-stack directory

rm -rf dist && ~/Code/zeit/ncc/dist/ncc/cli.js cache clean && ~/Code/zeit/ncc/dist/ncc/cli.js build index.ts --source-map && node dist

I still see the same (incorrect) error:

Error: Should throw
    at Server.handler (/Users/styfle/Code/foo/ncc-bug-stack/dist/index.js:61:19)
    at Server.emit (events.js:189:13)
    at parserOnIncoming (_http_server.js:676:12)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:109:17)

I even tried disabling chrome caching 🤷‍♂️

@styfle
Copy link
Member

styfle commented Apr 19, 2019

Ok so change tsconfig.json to enable sourceMap:true fixed the problem.

We'll have to make sure that user's enable this feature in their TS projects.

@styfle styfle merged commit d4aa166 into master Apr 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect source maps for typescript

3 participants