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(loader): source map sources and file paths #602

Closed

Conversation

npetruzzelli
Copy link

  • Improves source map path reliability (legacy and normal)
  • Fix sources and file to be URLs that are relative to the file containing the map.
    • Because of how dramatically different this is, the user must opt-in to this part of the fix by setting legacySourceMaps option to false. Making this fix the default behavior would be a breaking change requiring a new major version.

Side note: npm run test-source-map throws an error when using webpack 4.5+. I needed to install webpack 3.x for it to run correctly.

This contributes to the fixes for

…ns.file` and `options.outFile` (outFile does not cause anything to be be written to the file system.)

Signed-off-by: Nick Petruzzelli <code.npetruzzelli@gmail.com>
…ult out of sensitivity to existing scripts that may expect this behavior.

Signed-off-by: Nick Petruzzelli <code.npetruzzelli@gmail.com>
@jsf-clabot
Copy link

jsf-clabot commented Aug 3, 2018

CLA assistant check
All committers have signed the CLA.

@npetruzzelli
Copy link
Author

See also: webpack-contrib/css-loader#753

@npetruzzelli
Copy link
Author

I'm seeing some build errors that I did not experience locally. I'll have to look have another look.

Signed-off-by: Nick Petruzzelli <code.npetruzzelli@gmail.com>
… map.

Signed-off-by: Nick Petruzzelli <code.npetruzzelli@gmail.com>
@npetruzzelli
Copy link
Author

npetruzzelli commented Aug 4, 2018

Looks like:

  • I need to update an existing test
  • I need to add additional tests to meet threshold requirements.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

No new option, please provide minimum reproducible test repo, we should fix it without new options

@alexander-akait
Copy link
Member

Also you PR is breaking change, we can't accept this

Signed-off-by: Nick Petruzzelli <code.npetruzzelli@gmail.com>
@npetruzzelli
Copy link
Author

@evilebottnawi - I've removed the option part of it. I still need to update the test suite and create a test repo. It may take me some time to get to it. I'll comment again once it is all in place. You may see commits trickle in in the mean time.

I believe concerns about it being a breaking change may be addressed by then, if not, I may have more questions at that time.

… pass.

Signed-off-by: Nick Petruzzelli <code.npetruzzelli@gmail.com>
@codecov
Copy link

codecov bot commented Aug 13, 2018

Codecov Report

Merging #602 into master will decrease coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #602      +/-   ##
==========================================
- Coverage   97.81%   97.67%   -0.14%     
==========================================
  Files           6        6              
  Lines         137      129       -8     
==========================================
- Hits          134      126       -8     
  Misses          3        3
Impacted Files Coverage Δ
lib/loader.js 100% <ø> (ø) ⬆️
lib/normalizeOptions.js 95.83% <100%> (-0.6%) ⬇️

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 6085bf6...e9fea9b. Read the comment docs.

@michael-ciniawsky michael-ciniawsky changed the title Fix Source map sources and file fix: source map sources and file paths Aug 23, 2018
@michael-ciniawsky michael-ciniawsky changed the title fix: source map sources and file paths fix(loader): source map sources and file paths Aug 23, 2018
@michael-ciniawsky michael-ciniawsky added this to the 7.1.1 milestone Aug 23, 2018
@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #602 into master will decrease coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #602      +/-   ##
==========================================
- Coverage   97.79%   97.67%   -0.12%     
==========================================
  Files           6        6              
  Lines         136      129       -7     
==========================================
- Hits          133      126       -7     
  Misses          3        3
Impacted Files Coverage Δ
lib/loader.js 100% <ø> (ø) ⬆️
lib/normalizeOptions.js 95.83% <100%> (-0.6%) ⬇️
lib/proxyCustomImporters.js 100% <0%> (ø) ⬆️
lib/webpackImporter.js 100% <0%> (ø) ⬆️
lib/importsToResolve.js 100% <0%> (ø) ⬆️
lib/formatSassError.js 88.88% <0%> (+0.65%) ⬆️

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 2adcca3...e9fea9b. Read the comment docs.

@alexander-akait
Copy link
Member

Close due inactivity, if you still have problem please open issue, sorry for delay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants