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 sourcemaps errors and update test #26

Merged
merged 19 commits into from
Sep 16, 2016
Merged

Conversation

thekip
Copy link
Contributor

@thekip thekip commented Aug 30, 2016

This huge PR brings next improvments:

Tests

First of all i rewrite tests using Tape and change folder structure. It's more convinient way for testing, also such tests more flexible and can be debugged with Node debugger. Bash scripts is not cross-platform solution (Windows does'not support bash scripts without extra tools).

Besides i've added some new test cases. I'll describe this test cases later.

Issues

The main issue i've tried to solve, is duplicating files in sourcemaps (#24 see last comment, also #23).

But actually there is a bunch of things cause this issue.
Generally, problem is here:

var generator = SourceMapGenerator.fromSourceMap(new SourceMapConsumer(annotateMap));
generator.applySourceMap(new SourceMapConsumer(inputSourceMap), filename);

outputSourceMap = generator.toJSON();

inputSourceMap.sources should have same path and name as annotateMap.sources.

Otherwise outputSourceMap will be have duplicate files in sources.

Example:

image

As you can see in screenshot, outputSourceMap.sources has two entries, becuase sources in inputMap and annotateMap have diffrent slahes and path (relative/absoulte)

Therefore we see the first problem:

Sourcemap doesn't care about forward slahes

If you are using *nix system you have never got this error. See what mozilla/source-map team think about this mozilla/source-map#91

So solution is simple, we need normalize pathes everewhere.

There is a big problem with accepatance tests, we can't simulate this behaviour, it means somebody on unix system can rewrite something and tests will not report any problems. (i think we need to refactor loader, and write unit test for mergeMap() function)

Prefixed resource path

Sometimes webpack (or not?), emit map with sources prefixed by loader name:

node_modules\tslint-loader\index.js!c:\opensource\ng-annotate-loader\cases\typescript\file-to-annotate.ts

Actually i don't know when and why it happend. I spend a lot of time to find the reason. But all i found was awesome-typescript-loader + tslint as preloader processed '*.ts' file caused this behaviour.

In our project, eslint cuase the same when precessing *.js files.

I added special test case typescript which cover this issue.

I was looking for a solution for a long time, i've tried few diffrent approaches, but finally is very simple:

 var filename = normalizePath(this.resourcePath);
inputSourceMap.sourceRoot = '';
inputSourceMap.sources[0] = filename;

Source map in loader always have just one entry in sources and this entry the same to processed file. We override entry, with normalized resource path.
Some loaders return sources with relative path and filled sourceRoot. We clear sourceRoot, because we override path with absoulte url, and we don't need prefix.

Issues with sourceRoot

SourceMapGenerator can merge maps even if they have not absolutely the same sources.
It can merge relative with absoulte path but only if correct source root is provided.

Sourcemaps from ngAnnotate always have absolute path, but inputSourceMap.sources can be diffrent it depend on loader.

Sometimes it has relative path + sourceRoot (babel-loader), sometimes only relative path (typescipt-loader), sometimes absolute path.

Solution the same as in previous issue: explicit set absolute path and clear sourceRoot.

I've added test case uglifyjs which cover this issue.

Dependecies

I've updated all dependecies, and update test references (because babel6 generate diffrent output)
Also i've added .editorconfig to help writing code in same code style.

@thekip
Copy link
Contributor Author

thekip commented Aug 30, 2016

Test failed. Interesting. Webpack Hash in source maps is diffrent... need another way to check sourcemaps valid. Dirty checking files is not good solution.

@andrey-skl
Copy link
Owner

@thekip Great job! I need some time to get into it. Thank you for the PR!

@thekip
Copy link
Contributor Author

thekip commented Aug 30, 2016

Problem not only in hash (
Diffrent between unix/windows line endings.
I already fixed that in generated source, but the same mistake exist in maps as well, and it's not so easy to fix.

@thekip
Copy link
Contributor Author

thekip commented Sep 8, 2016

Last two commits bring next improvments:

  1. Bugfix: when ngAnnotate do nothing on file it still generate sourcemap (i don't know what exactly mapped inside), when this map merged with input map it produce new broken merged map.
    So i check, if source code are the same (nothing was to annotate) I just return input sourcemap without merging.
    This is old bug, but previously we didn't notice that because it breaks a duplicate file in maps.
  2. Change the way how maps are tested. I've created special file sourcemap-checkpoints.js for each test case, which used as reference for sourcemap.
    This is more intelegent way to test maps, because we exactly know work this map or not as if tested in browser.
    Also this fix issues with crlf/lf.

@hekike
Copy link

hekike commented Sep 16, 2016

Any progress on it?

@thekip
Copy link
Contributor Author

thekip commented Sep 16, 2016

This is ready to merge, wait for @huston007

@hekike You can use my fork, untill it merged to original packet.

@andrey-skl
Copy link
Owner

andrey-skl commented Sep 16, 2016

@thekip @hekike Oh that cool! Didn't notice that it started passing the build. Merging.

@andrey-skl andrey-skl merged commit a3ee26d into andrey-skl:master Sep 16, 2016
@hekike
Copy link

hekike commented Sep 16, 2016

@huston007 thanks ;)
When do you plan to release it?

@andrey-skl
Copy link
Owner

andrey-skl commented Sep 16, 2016

@hekike I have to perform some testing first. You could help me if you try and say it works for you : )

@thekip
Copy link
Contributor Author

thekip commented Sep 16, 2016

@huston007 I use it about half month in our huge project (codebase about 3mb) and it works. My teammates didn't report any issues related to that.

You can run it on yours project, and test it as well.

@andrey-skl
Copy link
Owner

@thekip Yep I will. Thank you for the job you've done.

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.

3 participants