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

Fix T2864 #3108

Merged
merged 2 commits into from Dec 3, 2015

Conversation

Projects
None yet
6 participants
@cspotcode
Copy link
Contributor

cspotcode commented Nov 24, 2015

This PR fixes issue 2864 as well as another, minor mistake.

Babel doesn't generate mappings for leading comments, but it assumes its own sourcemap can map any input position. Other tools, such as Typescript, do create mappings for leading comments. Babel's sourcemap can't handle those mappings, hence the "Invalid mapping." Fortunately, it's fine to omit them, which is what my patch does.

The second commit fixes the source path being passed to generatedPositionFor. The output sourcemap's file and source[0] were, coincidentally, both "unknown", which is why the mistake wasn't causing a bug. I still think this change is warranted since it clarifies the code and avoids a possible future bug.

Let me know if I need to reformat for code style or split this into two PRs. Thanks!

cspotcode added some commits Nov 24, 2015

Fixes T2864
- Drops input mappings that cannot be mapped through Babel's sourcemap
- For example, Babel's sourcemap does not have mappings for leading comments, so any mapping from an input sourcemap for those leading comments must be dropped from the merged sourcemap
Fixes the wrong source path being passed to SourceMapConsumer#generat…
…edPositionFor

`generatedPositionFor` accepts a position in one of the sourcemap's *input* source files. Therefore the `source` path
should be one of the sourcemap's input `sources`, not the output `file`.
@johnnyreilly

This comment has been minimized.

Copy link

johnnyreilly commented Nov 24, 2015

Awesome! Hope this gets merged soon.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 24, 2015

Current coverage is 89.14%

Merging #3108 into master will decrease coverage by -0.03% as of b4cd186

@@            master   #3108   diff @@
======================================
  Files          214     214       
  Stmts        15468   15471     +3
  Branches         0       0       
  Methods          0       0       
======================================
- Hit          13793   13792     -1
  Partial          0       0       
- Missed        1675    1679     +4

Review entire Coverage Diff as of b4cd186

Powered by Codecov. Updated on successful CI builds.

@johnnyreilly

This comment has been minimized.

Copy link

johnnyreilly commented Nov 25, 2015

I gave this a quick test locally and I had a minor issue with my initial setup throwing this error:

ERROR in ./src/dependencies.ts
Module build failed: TypeError: C:/source/TypeScript-ES6-Babel-React-Flux-Karma/src/dependencies.ts: Cannot read propert
y 'indexOf' of undefined
    at Object.relative (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\source-map\lib\util.js:206:17)
    at SourceMapConsumer_generatedPositionFor [as generatedPositionFor] (C:\source\TypeScript-ES6-Babel-React-Flux-Karma
\node_modules\source-map\lib\source-map-consumer.js:730:23)
    at C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\file\index.js:443:45
    at Array.forEach (native)
    at SourceMapConsumer_eachMapping [as eachMapping] (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\sour
ce-map\lib\source-map-consumer.js:155:16)
    at C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\file\index.js:442:18
    at File.mergeSourceMap (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\f
ile\index.js:483:9)
    at File.generate (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\file\in
dex.js:677:25)
    at File.transform (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\file\i
ndex.js:524:17)
    at C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\pipeline.js:46:19
    at File.wrap (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\file\index.
js:534:16)
    at Pipeline.transform (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\pi
peline.js:43:17)
    at transpile (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-loader\index.js:14:22)
    at Object.module.exports (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-loader\index.js:87:14)
 @ ./src/main.tsx 3:0-25

It turned out this was down to me having a file called dependencies.ts which only contained the Babel polyfill:

import 'babel-polyfill';

i.e. No actual code itself. (I believe I have this in place so Karma can use babel-polyfill in each of the unit tests)

However, once I moved the import 'babel-polyfill'; into the main.tsx that seemed to resolve the issues. (There's no unit tests for main.tsx; it's basically a bootstrapper)


Anyway - minor niggle aside this looks good to me. Hope it is merged soon. Thanks @cspotcode for your work!

@cspotcode

This comment has been minimized.

Copy link
Contributor Author

cspotcode commented Nov 25, 2015

I'd definitely like to fix this before merging. Also, I want to verify
that the merged sourcemap can map properly to both the original source (eg
.ts) and the intermediate source (eg .es6 file generated by tsc). For
example, when Typescript inlines helper functions into its output, the
merged sourcemap must map to the intermediate .es6 since those helper
functions don't exist in the .ts file.

By the way, does babel have an option to run with
require('source-map-support') so that those stack traces show source
positions instead of transpiled positions? It would be really helpful for
debugging.
https://www.npmjs.com/package/source-map-support
On Nov 25, 2015 2:12 AM, "John Reilly" notifications@github.com wrote:

I gave this a quick test locally and I had a minor issue with my initial
setup throwing this error:

ERROR in ./src/dependencies.ts
Module build failed: TypeError: C:/source/TypeScript-ES6-Babel-React-Flux-Karma/src/dependencies.ts: Cannot read propert
y 'indexOf' of undefined
at Object.relative (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\source-map\lib\util.js:206:17)
at SourceMapConsumer_generatedPositionFor [as generatedPositionFor](C:sourceTypeScript-ES6-Babel-React-Flux-Karma
node_modulessource-maplibsource-map-consumer.js:730:23)
at C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\file\index.js:443:45
at Array.forEach (native)
at SourceMapConsumer_eachMapping [as eachMapping](C:sourceTypeScript-ES6-Babel-React-Flux-Karmanode_modulessour
ce-maplibsource-map-consumer.js:155:16)
at C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\file\index.js:442:18
at File.mergeSourceMap (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\f
ile\index.js:483:9)
at File.generate (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\file\in
dex.js:677:25)
at File.transform (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\file\i
ndex.js:524:17)
at C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\pipeline.js:46:19
at File.wrap (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\file\index.
js:534:16)
at Pipeline.transform (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-core\lib\transformation\pi
peline.js:43:17)
at transpile (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-loader\index.js:14:22)
at Object.module.exports (C:\source\TypeScript-ES6-Babel-React-Flux-Karma\node_modules\babel-loader\index.js:87:14)
@ ./src/main.tsx 3:0-25

It turned out this was down to me having a file called dependencies.ts
which only contained the Babel polyfill:

import 'babel-polyfill';

i.e. No actual code itself. (I believe I have this in place so Karma can
use babel-polyfill in each of the unit tests)

However, once I moved the import 'babel-polyfill'; into the main.tsx that
seemed to resolve the issues. (There's no unit tests for main.tsx; it's

basically a bootstrapper)

Anyway - minor niggle aside this looks good to me. Hope it is merged soon.
Thanks @cspotcode https://github.com/cspotcode for your work!


Reply to this email directly or view it on GitHub
#3108 (comment).

@davidreher

This comment has been minimized.

Copy link

davidreher commented Dec 3, 2015

please merge this soon, this stops me from migrating to babel6 ...

});
if(generatedPosition.column != null) {

This comment has been minimized.

@hzoo

hzoo Dec 3, 2015

Member

Do we want to get line or source to be null as well?

This comment has been minimized.

@cspotcode

cspotcode Dec 3, 2015

Author Contributor

I'm assuming that either
a) both column and line are numbers, meaning a mapping was found
b) both are null, meaning a mapping couldn't be made.

Is there a situation where only one would be null?

This comment has been minimized.

@hzoo

hzoo Dec 3, 2015

Member

I'm not sure (don't know too much about this) - but your assumption makes sense since the two are tied together.

And source won't be null since we are passing that in from outputMapConsumer.sources[0];?

This comment has been minimized.

@cspotcode

cspotcode Dec 3, 2015

Author Contributor

Yes, it's my understanding that all sources need a name, so source will never be null if a mapping is found.

@@ -359,21 +359,28 @@ export default class File extends Store {
sourceRoot: inputMapConsumer.sourceRoot
});

// This assumes the output map always has a single source, since Babel always compiles a single source file to a
// single output file.
const source = outputMapConsumer.sources[0];

This comment has been minimized.

@hzoo

hzoo Dec 3, 2015

Member

If this is true - this should be inputMapConsumer correct?

or can we just use mapping.source from the function param like in

mergedGenerator.addMapping({
            source: mapping.source,
...

This comment has been minimized.

@cspotcode

cspotcode Dec 3, 2015

Author Contributor

The code is correct. source is being passed to outputMapConsumer.generatedPositionFor. The first argument to that function must describe a position in one of outputMapConsumer's source files, not inputMapConsumer's or anything else. Hence, we grab outputMapConsumer.sources[0].

mapping.source refers to the source file of the input map. However, the output map's source correspond to the input map's generated code.

This comment has been minimized.

@hzoo

hzoo Dec 3, 2015

Member

Ok thanks for the explanation - looks good!

hzoo added a commit that referenced this pull request Dec 3, 2015

@hzoo hzoo merged commit c8a6d81 into babel:master Dec 3, 2015

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@johnnyreilly

This comment has been minimized.

Copy link

johnnyreilly commented Dec 3, 2015

So pleased to see this get merged! @cspotcode is it your original fix that I tested that was merged or did you do the extra fix you were talking about here:

I'd definitely like to fix this before merging.

@cspotcode

This comment has been minimized.

Copy link
Contributor Author

cspotcode commented Dec 3, 2015

@johnnyreilly: I have not done the extra work I was talking about. I'm hoping to work on it today. Can you post a minimal reproduction of the problem you were having, so I can use it for testing?

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Dec 3, 2015

Oh I must of missed that comment

@cspotcode

This comment has been minimized.

Copy link
Contributor Author

cspotcode commented Dec 3, 2015

No worries, merging this patch is still an improvement.

Regarding:

Also, I want to verify that the merged sourcemap can map properly to both the original source (eg .ts) and the intermediate source (eg .es6 file generated by tsc).

I just confirmed that the sourcemap does not map to intermediate sources. I suppose it's debatable whether or not it should; I think it should.

@johnnyreilly

This comment has been minimized.

Copy link

johnnyreilly commented Dec 3, 2015

Sure @cspotcode. Clone this: https://github.com/johnnyreilly/TypeScript-Babel-6-Problems

Once you've

npm install
npm run watch

You should see the error being thrown by webpack in the console. If you actually have something to import in main.ts:

https://github.com/johnnyreilly/TypeScript-Babel-6-Problems/blob/master/src/main.ts#L1

Then the code would work (assuming you're using the version that's just been merged). Without it, it doesn't. Apologies this this isn't more minimal but I'm time poor right now. Hopefully this is enough to get you going.

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Dec 5, 2015

Interesting - https://phabricator.babeljs.io/T6766 - it looks like the mappings is empty for the modules plugins that aren't commonjs.

@johnnyreilly

This comment has been minimized.

Copy link

johnnyreilly commented Dec 10, 2015

Did that work as a repro for you @cspotcode?

@cspotcode

This comment has been minimized.

Copy link
Contributor Author

cspotcode commented Dec 14, 2015

@johnnyreilly I haven't had a chance to look at it, sorry. If anyone else wants to tackle this issue, go for it. If not, I'll hopefully be able to look at it sometime after the holidays.

@johnnyreilly

This comment has been minimized.

Copy link

johnnyreilly commented Dec 14, 2015

Thanks chap. I don't really understand how sourcemaps work and don't have the time right now to learn. If you can look at it at some point it would be fantastic. But I don't think it's particularly urgent. (The current set up works!)

@mstade

This comment has been minimized.

Copy link

mstade commented Dec 23, 2015

This may have caused T6766. FWIW I've created a repro case as well, I hope it helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.