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

Remove extra source map file + write source maps only if they are enabled #453

Merged
merged 2 commits into from Mar 13, 2017

Conversation

bebraw
Copy link
Contributor

@bebraw bebraw commented Mar 13, 2017

What kind of change does this PR introduce?

Refactoring.

Did you add tests for your changes?

Old tests should work (had to tweak one).

Summary

This gets rid of the older hackfix.

@codecov
Copy link

codecov bot commented Mar 13, 2017

Codecov Report

Merging #453 into master will increase coverage by 7.48%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master     #453      +/-   ##
==========================================
+ Coverage    90.7%   98.19%   +7.48%     
==========================================
  Files          10        9       -1     
  Lines         398      332      -66     
  Branches       85       75      -10     
==========================================
- Hits          361      326      -35     
+ Misses         37        6      -31
Impacted Files Coverage Δ
lib/loader.js 100% <100%> (ø)
lib/css-base.js 95% <92.3%> (-2.15%)

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 ee7234b...3122e43. Read the comment docs.

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

Nice 👍 :shipit:

Copy link

@fklingler fklingler left a comment

Choose a reason for hiding this comment

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

It seems to work for me, fixing the problem I was talking about here: #447 (comment) 👍 :shipit:

@bebraw bebraw merged commit 4250129 into master Mar 13, 2017
@bebraw
Copy link
Contributor Author

bebraw commented Mar 13, 2017

Published in 0.27.3. Thanks for the confirmation.

@michael-ciniawsky michael-ciniawsky deleted the fix-sourcemap branch March 13, 2017 15:45
@fklingler
Copy link

Ok I've got some bad news... I just realized it doesn't work when the sourceMap option is present.

I have to set

  node: {
    Buffer: true
  }

in the webpack options for this to be working.

@michael-ciniawsky
Copy link
Member

@fklingler That's confusing in #454 the opposite seems to be the case :D, can you show the bundle output and maybe make a small test repo for reproduction, there is a small bug somewhere. Please move the discussion to #454 aswell to keep it at one place 😛

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.

None yet

3 participants