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

feat: Include the sourceMappingURL & sourceURL when toString() #417

Merged

Conversation

asnowwolf
Copy link
Contributor

What kind of change does this PR introduce?

feature

Did you add tests for your changes?

yes

If relevant, did you update the README?

no

Summary

This feature is useful when you need to pass the result of css-loader to angular: call to require('to-string-loader!css-loader? sourceMap!sass-loader?sourceMap!./ test.scss') will include source mapping, And finally assigned to the component's styles metadata

Does this PR introduce a breaking change?

I think no, Unless some code assumes that the css content does not have source maps

Other information

It's useful to angular component style.

@asnowwolf asnowwolf force-pushed the include-source-mapping branch 3 times, most recently from 202b58d to 18a066e Compare February 5, 2017 10:37
@asnowwolf
Copy link
Contributor Author

asnowwolf commented Feb 5, 2017

I did not receive the e-mail for sign CLA, could you please re-send it? @sokra @d3viant0ne @kennyt

@joshwiens
Copy link
Member

@asnowwolf - Close & Open a new Pull Request. That should trigger the CLA system to send it again.

@asnowwolf asnowwolf closed this Feb 8, 2017
@asnowwolf asnowwolf reopened this Feb 8, 2017
@codecov
Copy link

codecov bot commented Feb 8, 2017

Codecov Report

Merging #417 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #417      +/-   ##
=========================================
+ Coverage   98.36%   98.4%   +0.04%     
=========================================
  Files           9       9              
  Lines         306     314       +8     
  Branches       69      71       +2     
=========================================
+ Hits          301     309       +8     
  Misses          5       5
Impacted Files Coverage Δ
lib/css-base.js 97.14% <100%> (+0.84%)

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 476a927...23eea3d. Read the comment docs.

@asnowwolf
Copy link
Contributor Author

I tried, but still no email sent to me... does the bot work properly?

@asnowwolf
Copy link
Contributor Author

asnowwolf commented Feb 8, 2017

Could you please send me a CLA mail and I manually sign it?

@michael-ciniawsky
Copy link
Member

@asnowwolf Can you please try again the CLA Bot should work now 😛

@joshwiens
Copy link
Member

@asnowwolf - You will need to close & reopen this pull request to trigger the CLA assistant so it can be signed.

@ekulabuhov
Copy link
Member

Is toString() a part of the public API? Should it be documented?

@asnowwolf asnowwolf closed this Mar 9, 2017
@asnowwolf asnowwolf reopened this Mar 9, 2017
@jsf-clabot
Copy link

jsf-clabot commented Mar 9, 2017

CLA assistant check
All committers have signed the CLA.

@asnowwolf
Copy link
Contributor Author

@ekulabuhov oh, yes, it is. I'll try to fix the documents too.

@asnowwolf asnowwolf force-pushed the include-source-mapping branch 2 times, most recently from 45dfe63 to 4b9ac2e Compare March 9, 2017 00:44
@asnowwolf
Copy link
Contributor Author

@ekulabuhov

Done.

But my english is not so good, please review it and modify it freely.

Thank you!

@asnowwolf
Copy link
Contributor Author

If this PR is published to npm, please @ me here, some of other PRs are waiting for this one.

This feature is useful when you need to pass the result of css-loader to angular: call to require('to-string-loader!css-loader? sourceMap!sass-loader?sourceMap!./ test.scss') will include source mapping, And finally assigned to the component's styles metadata
@asnowwolf asnowwolf force-pushed the include-source-mapping branch from 4b9ac2e to 23eea3d Compare March 9, 2017 00:50
@joshwiens
Copy link
Member

@michael-ciniawsky - This should be removed from the 1.0.0 milestone and published as a Minor as it's backwards compatible and beneficial to the community now.

Updating to defaults will take some time, no reason to hold this feature up.

@asnowwolf
Copy link
Contributor Author

Oh, so... a standalone loader, eg. https://github.com/asnowwolf/css-with-mapping-loader, is more preferred?

@joshwiens
Copy link
Member

@asnowwolf - No, that was just a comment on how we are going to release this to npm.

The pull request will be accepted & released before the coming semver MAJOR release that is in planning.

@asnowwolf
Copy link
Contributor Author

Oh, I see!

@michael-ciniawsky
Copy link
Member

@d3viant0ne Alright, yep the v1.0.0 Project is more for sorting, when it's getting real I definitely make a Milestone with timeline :D

@michael-ciniawsky michael-ciniawsky merged commit c372543 into webpack-contrib:master Mar 9, 2017
@michael-ciniawsky
Copy link
Member

@asnowwolf Thx 😛

@ekulabuhov
Copy link
Member

@asnowwolf Thank you! That's perfect 👍

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.

5 participants