fix: source map sources and file paths
#753
Conversation
…ult out of sensitivity to existing scripts that may expect this behavior. Signed-off-by: Nick Petruzzelli <code.npetruzzelli@gmail.com>
|
|
|
See also: webpack-contrib/sass-loader#602 |
Signed-off-by: Nick Petruzzelli <code.npetruzzelli@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #753 +/- ##
==========================================
+ Coverage 99.45% 99.74% +0.28%
==========================================
Files 10 10
Lines 370 388 +18
Branches 104 90 -14
==========================================
+ Hits 368 387 +19
+ Misses 2 1 -1Continue to review full report at Codecov.
|
|
Looks like the appveyor build is broken. |
Signed-off-by: Nick Petruzzelli <code.npetruzzelli@gmail.com>
|
@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. |
lib/processCss.js
Outdated
| pipeline.process(inputSource, { | ||
| from: postCssFrom, | ||
| from: options.from, |
There was a problem hiding this comment.
Break maps in many cases
There was a problem hiding this comment.
Are you referring to the existing test suite? It is expected that it broke. I held off on updating the test suite as the comments in this review would determine how it would need to be updated.
The existing test suite is still using assumptions about source maps prior to the fix in this PR. I currently have the feedback I need and I intend to update the test suite prior to setting up the test repo.
If it isn't the test suite, is there a specific scenario you can describe that I can look into?
michael-ciniawsky
changed the title
sources and file pathssources and file paths
|
First thx @npetruzzelli for the effort, but before we continue implementing we need to discuss the source map story as a whole. I had mutliple issue(s) about source maps since I'm maintaining these loaders and there were fixes provided that solved issues for some folks while adding new problems for others, it either is something which needs to be configurated for some cases or we just not getting it right atm... 😛 |
|
@michael-ciniawsky - I am happy to discuss it. If I wanted a solution to work just for me I could publish a fork under a new namespace. My goal is to fix it for everyone. I see source maps as a valuable tool and these loaders will be the only option available to many who don't have the time/budget to learn how to make build systems from the ground up. Can you share with me what the past issues were? The fix I'm envisioning sticks as close as possible to the source maps v3 spec. That said, if it breaks the tool for many users, then it isn't helping. If the use cases that need separate configuration can be clearly defined, I think it is a problem we can solve. I am simply starting with css-loader and sass-loader. I can't speak for the JS source maps (yet), but I intend to touch more loaders and plugins related to the stylesheet source maps. The problem is not something you can point at one loader or plugin and say it is the source of the problem, at least with what I have found so far with css, sass, and postcss loaders. I'm not interested in pointing fingers, so I apologize if my choice of words seem harsh. I just want to help fix it! |
I cause my comment sounds like if I implied your currently trying to do that, that's not the case
Sure, I will try to look some of them up, it was always about paths being incorrect (relative to the file in the wrong way etc) in a back and forth manner
The fix you propose in this PR already looks familiar to me (setting |
To clear things up, I didn't mean to make that implication! My PR effectively comes in and says that a past contributor got something wrong. My goal isn't to put that person down, I am here to help solve a challenge. I may have been overly self-conscious.
I'm having difficulty figuring out what I hope to more time into the pull requests for
I took a quick look at postcss-loader and the related part of PostCSS's code. Even if this kind of trick works right now, there is no guarantee that it will work in the future as it is an undocumented implementation detail. PostCSS's own documentation is what says to set both values to the same path. At least for when I've used the Autoprefixer PostCSS plugin, I've always received expected source maps when providing both |
|
Hello guys, I've just found this PR today, just before I create a new issue exactly about this )) import st from './assets/styles/main.styl';I get this paths double path bug |
|
Although ! maybe I don't understand that PR correctly and my problem related to behavior of stylus-loader ... just tested in my repo, and my problem still there |
Signed-off-by: Nick Petruzzelli <code.npetruzzelli@gmail.com>
|
Appveyor is failing in a way that I don't believe is related to the pull request. TravisCI is failing because @evilebottnawi - I've updated the test suite. My next task is to setup a test repo. I plan to take another pass at the test suite after feedback. Do you have any specific criteria you would like to see in the test repo? @michael-ciniawsky - Have I addressed the concerns for this repository? (not including updates other loaders or plugins, I intend to handle those within their respective repos). I'll be happy to continue the conversation! |
lib/loader.js
Outdated
| * system. They don't know about, care about, or understand the webpack | ||
| * loader's current request or remaining request. | ||
| */ | ||
| processCssFrom = processFrom(this.resourcePath, map); |
There was a problem hiding this comment.
var from = getFrom(this.resourcePath, map)
var to = from
lib/loader.js
Outdated
| processCss(content, map, { | ||
| mode: moduleMode ? "local" : "global", | ||
| from: loaderUtils.getRemainingRequest(this).split("!").pop(), | ||
| to: loaderUtils.getCurrentRequest(this).split("!").pop(), | ||
| from: processCssFrom, |
There was a problem hiding this comment.
{
...,
// TODO use shorthand in next major
from: from,
to: to
}
lib/loader.js
Outdated
| */ | ||
| function processFrom(resourcePath, map) { | ||
| var effectiveResourcePath; | ||
| if (map && map.file && typeof map.file === 'string' && path.dirname(map.file) === '.') { |
There was a problem hiding this comment.
path.dirname(map.file) === '.' checks for a relative path here ? I'm not sure I'm following.. 😛 . In case my understanding is correct here, there is also path.isAbsolute(filepath) available
There was a problem hiding this comment.
I've done some thinking on it and here I may be trying to be too clever. What I originally was thinking of shouldn't be something that css-loader is responsible for. I can provide more details on what I was thinking if you'd like.
I'll try cleaning this up in my next commit.
from: this.resourcePath,
to: this.resourcePath,will be enough.
lib/loader.js
Outdated
| if (map && map.file && typeof map.file === 'string' && path.dirname(map.file) === '.') { | ||
| // Something else has already changed the file name or extension, so | ||
| // honor it for the purpose of creating the next source map. | ||
| effectiveResourcePath = path.join(path.dirname(resourcePath), map.file); |
There was a problem hiding this comment.
Please provide an example for effectiveResourcePath here :)
lib/loader.js
Outdated
| effectiveResourcePath = path.join(path.dirname(resourcePath), map.file); | ||
| } else { | ||
| effectiveResourcePath = resourcePath; | ||
| } |
There was a problem hiding this comment.
nitpick 🐦 \n after the else block
|
@michael-ciniawsky - I'll try to have complete responses for you in the next day or so. Sorry for the delay! |
|
I'm in the process of merging in the most recent updates to: I think I was able to carry over the bulk of my changes as a part of resolving the merge conflicts, but changes to unit testing means I have to update the test suite again. Once I update the test suite, I will be able to confirm whether or not this PR is still working as intended. I'm used to working with Mocha/Chai/Sinon. It may take a bit for me to wrap my head around Jest. |
|
@npetruzzelli Great! If you have problems with jest, send a PR without tests, we can write this late 👍 |
…t set it. Update test snapshots to reflect this.
|
I am still working on this, but found a couple things that are confusing me: // lib/loader.js
/* eslint-disable no-param-reassign */
if (sourceMap) {
if (map) {
if (typeof map === 'string') {
map = JSON.stringify(map);
}1. Why is
|
We can refactor this logic. Also this place for buggy loaders.
Some loader pass string, some objects. Also just compatibility. |
| to: getCurrentRequest(this) | ||
| .split('!') | ||
| .pop(), | ||
| from: resourcePath, |
There was a problem hiding this comment.
It is dangerous using this, also it is invalid use resourcePath, because it is not resource path, it is request, maybe you can create problem repository?
There was a problem hiding this comment.
It will take me a while to completely respond to this. If I am lucky, I can have something ready some time this week.
I need to understand more about the differences between when to use the request and when to use a file system path.
It is difficult to learn the details of the request syntax, because it does not exist on the website (https://webpack.js.org/concepts/) nor the documentation used to generate it (https://github.com/webpack/webpack.js.org). The closest we come is documentation on context: https://webpack.js.org/api/loaders/#the-loader-context.
There are some pages in the old wiki, but they don't go into a lot of detail and users looking for documentation might not expect to look in the deprecated docs repository wiki because they appear to have the same information at first glance and would expect the syntax to be documented on the site. In reality, the wiki contains information not present in the in the more up to date documentation: https://github.com/webpack/docs/wiki/loaders, https://github.com/webpack/docs/wiki/using-loaders.
The next post will probably be a big one.
Before I make it, I want to see if what I'm concerned about is still an issue or if I'm worried about nothing. My next step is to create the example repository that you requested as it will help illustrate what has been fixed and what still needs work. I can't rely only on the test suite just yet.
There was a problem hiding this comment.
@npetruzzelli feel free to create minimum reproducible test repo, it is allow to me also identify the problem and how to fix it
There was a problem hiding this comment.
Hi @evilebottnawi,
I've created the repository that illustrates the problem. I'll need some time to get some text and images together to support/describe it clearly instead of needing you to dig through multiple pull requests and issues across multiple repositories for a clear picture.
I plan to use this repository to demonstrate the eventual solution just by pointing to a different Webpack file, probably in a new branch.
Relevant information:
- OS: Windows 10 Pro 64-bit
- NODE: v8.9.4
- NPM: 6.4.1 (see also: npm-windows-upgrade)
- Windows Build Tools: 5.0.0
There was a problem hiding this comment.
@npetruzzelli great, thanks, i will see tomorrow, very tired today
There was a problem hiding this comment.
No rush! I know that feeling. Rest well.
There was a problem hiding this comment.
Reproducible test repo is not very simple, i try around 30 minutes to understand structure. Also looks many things broken, js source maps also doesn't work in you example. Please simplify example.
|
The following is a diagram illustrating where in PostCSS that code relating to source maps may be found. I may be using it as a reference later on in this code review. https://github.com/postcss/postcss/tree/046ab11f1be0eb3dd62bb3ea331134119168af11 |
Valid
|
PostCSS's
|
|
I know how |
|
@evilebottnawi : It looks like the master branch has been updated and the file structure has changed again. I'll need time to handle merge conflicts. Here are some notes for the demonstration repository that I eventually plan to add to that repository's README: NPM Scripts:
Expected / ActualWhen running the server and inspecting the paragraph of red lorem ipsum text, and examining the Note: the difference in port numbers are intentional.
|
|
Please note that the demonstration repo does not yet include webpack configurations that demonstrate a fix. |
|
For the sake of definition the
For the sake of definition the
For the sake of definition the
This directory is often considered to be a secondary source for an HTTP server's document root directory in development environments. For the sake of definition the
For the sake of definition the
Pretty much anything that helps you build and maintain your application, but isn't a part of the application itself would go here. In some cases, this may include additional application back end servers, API servers, or databases meant to support the front end in a development environment (if they are not a part of their own, separate code repository) |
|
Updated by: The new commit for the demo repo's master branch is: |
|
@npetruzzelli thanks, we fix it in near future, many works right now, sorry for delay |
|
/cc @npetruzzelli friendly ping, can we rebase? |
|
@evilebottnawi - I can try to set some time aside to rebase this branch and resolve any conflicts. I wasn't sure (based on the last comment in December) if you were working on a separate fix. I apologize for not seeking clarification. Once I am caught up, I'll try to see if the demo repo can be updated/improved as well. Either way it shouldn't delay the rebase. I'll try to tackle it at some point in the next few weeks. If I'm lucky, I may have time on Sunday to work on this. |
|
@evilebottnawi - I've begun working on updating this branch, but I've run into an error on commit: I'll start investigating the error, but I was wondering if you might know how to resolve it. It is preventing me from committing. |
|
@npetruzzelli remove node_modules and reinstall their |
|
@evilebottnawi - That was the first thing I tried. I tried it again just to be certain, but no luck. I tried the following:
It appears to be working now, though it looks like there is a new commit message syntax that I'll have to follow, that is easy enough to do. I'll have a look at the CONTRIBUTING markdown. I suspect the issue may have been in the generated |
|
The main goal of this PR it testing source maps on |
|
Looks we need do this manually and add tests step by step |
|
Done in #901, will be released in near future, feel free to tests and if you faced with issue feel free to create new issue with reproducible test repo, thanks for PR |
What kind of change does this PR introduce?
half bugfix / half feature
Did you add tests for your changes?
Not yet, but they will be similar to existing tests. Waiting for feedback/comments.
If relevant, did you update the README?
Yes
Summary
The
sourcesandfileentries in sourcemaps are URLs relative to the file contains the source map (.cssor.map) per version 3 of the source map specification.For many loaders concerned with stylesheets, they currently try to treat them as file system paths, which has been a significant factor in errors that appear in these paths, especially when paths are being resolved without first checking if they are already absolute. These file system paths often end being relative to something other than the file containing the map.
"Under the hood" this loader uses PostCSS, which actually doesn't know about webpack's current request and remaining request. It only cares about file system paths. Part of the fix involves setting both the
from(unmodified) and thetoargument to beingloader.resourcePath.Using file system paths in maps is possible, but it is best left to the plugin responsible for writing the source map to the css file or the map file. The
style-loaderwill have some additional considerations that are unique to it.Once
css-loaderandsass-loaderare fixed,postcss-loaderand others will eventually need PRs as well.Because the fix is a breaking change, but people may want to use it immediately (such as testing it with other loaders needing similar fixes), it has been implemented as a new option.
Does this PR introduce a breaking change?
No, but the intent is a future major version could make the opt-in fix the default behavior.
Other information
This contributes to the fixes for