Skip to content

fix options.sourceFileName gennerate bug#260

Closed
creeperyang wants to merge 2 commits intobabel:masterfrom
creeperyang:master
Closed

fix options.sourceFileName gennerate bug#260
creeperyang wants to merge 2 commits intobabel:masterfrom
creeperyang:master

Conversation

@creeperyang
Copy link
Contributor

One corner usage:

sourceRoot: '/Users/me/work/SugarDev',
filename: '/private/tmp/wow/app.js'

// with previous path.relative
sourceFileName: '../../../../private/tmp/wow/app.js'

It seems just okay, but when webpack use webpack.optimize.UglifyJsPlugin with sourceMap, compilation will run into error. Because webpack.optimize.UglifyJsPlugin will try to access source map via '/private/tmp/wow/app.js'.

More info see webpack/webpack#2616

So we should fix the relative.

webpack use source-map handle source map, and source-map use its own relative: https://github.com/mozilla/source-map/blob/master/lib/util.js#L188-L226

Here I just replace path.relative with source-map's util.relative, and It will fix this error.

@joshwiens
Copy link
Contributor

joshwiens commented Aug 6, 2016

@creeperyang -

1.) Can you get this rebased and up to date so we can get it reviewed and landed?
2.) Add a test to cover the new relative function.

@creeperyang
Copy link
Contributor Author

@d3viant0ne okay, I will do it.

@codecov-io
Copy link

codecov-io commented Aug 7, 2016

Current coverage is 81.69% (diff: 100%)

Merging #260 into master will increase coverage by 1.69%

@@             master       #260   diff @@
==========================================
  Files             5          6     +1   
  Lines           140        153    +13   
  Methods          21         22     +1   
  Messages          0          0          
  Branches         30         33     +3   
==========================================
+ Hits            112        125    +13   
  Misses           13         13          
  Partials         15         15          

Powered by Codecov. Last update a8b9189...ea82899

@shankie-codes
Copy link

Any reason we can't get this merged? This is causing me some pretty serious issues at the moment!

Massive thanks to @creeperyang for sorting this! I would have never though of this...

@NickStefan
Copy link

@creeperyang I also would love to see this merged. Is this PR still alive?

@joshwiens
Copy link
Contributor

@NickStefan - I'll give him a another day to respond and if we don't hear anything, i'll get in in directly.

@creeperyang
Copy link
Contributor Author

creeperyang commented Nov 8, 2016

Yeah, still alive. (conflicts resolved).

danez pushed a commit that referenced this pull request Nov 8, 2016
* improve sourceFileName gennerate logic with new relative method

* add test for relative method
@danez
Copy link
Member

danez commented Nov 8, 2016

Merged in 501d60d

@danez danez closed this Nov 8, 2016
This was referenced Dec 7, 2016
@danez
Copy link
Member

danez commented Dec 7, 2016

@creeperyang I kind of reverted this change and fixed it in a different way. This PR broke some cases on windows. #334 #339
I reproduced your initial problem with v6.2.7, and still cannot reproduce it with v6.2.9. If you have problems again, please open a ticket with a reproducible case, but let's hope this is fixed forever now 😇

Ognian pushed a commit to Ognian/babel-loader that referenced this pull request Feb 27, 2017
* improve sourceFileName gennerate logic with new relative method

* add test for relative method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants