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

Sourcemap paths produced by rework are invalid on Windows #98

Closed
rjgotten opened this issue Aug 29, 2018 · 19 comments
Closed

Sourcemap paths produced by rework are invalid on Windows #98

rjgotten opened this issue Aug 29, 2018 · 19 comments

Comments

@rjgotten
Copy link

rjgotten commented Aug 29, 2018

Rework generates bad source map paths on Windows.

Given the piece of logic that calls out to rework:

resolve-url-loader/index.js

Lines 110 to 115 in 7105d8a

reworked = rework(contentWithMap, {source: loader.resourcePath})
.use(reworkPlugin)
.toString({
sourcemap : options.sourceMap,
sourcemapAsObject: options.sourceMap
});

If I add some logging of the source map before and after, then here's what I get.

The 'before' from absSourcemap (which is stringified into contentWithMap):

[
  "D:/path-to-project-on-disk/src/styles/main.scss",
  "D:/path-to-project-on-disk/src/styles/_icons.scss",
  "D:/path-to-project-on-disk/src/styles/_typography.scss",
  // ...
]

The 'after' from reworked.map.sources:

[
  "/path-to-project-on-disk/src/styles/main.scss",
  "/path-to-project-on-disk/src/styles/D:/path-to-project-on-disk/src/styles/_icons.scss",
  "/path-to-project-on-disk/src/styles/D:/path-to-project-on-disk/src/styles/_typography.scss",
  // ...
]

Not sure what's going on in there, but something in that library you're depending on is definitely broken as hell.

@bholloway
Copy link
Owner

@rjgotten please refer to #97. There has been a lot of effort on a @next version that is close to release.

I am hoping that it addresses your issue. Certainly it has automated tests in windows that cover source-map sources.

@rjgotten
Copy link
Author

rjgotten commented Aug 29, 2018

@bholloway
Hmm... From a (admittedly very cursory) look at things, this doesn't just fix the corrupt path problem in resolve-url-loader, but even prevents the same problem from re-occuring in postcss-loader further up the loader stack, as per webpack-contrib/postcss-loader#390

If so: ni----ce. 👍

@bholloway
Copy link
Owner

bholloway commented Aug 29, 2018

@rjgotten I made this comment some time ago. Is it the same issue in postcss-loader or something new?

@rjgotten
Copy link
Author

rjgotten commented Aug 29, 2018

@bholloway
It's that same thing.

One problem with the @next version of resolve-url-loader though.
While it appears that my sourcemap paths are now (more) correct, their content has gone missing.

@bholloway
Copy link
Owner

bholloway commented Aug 29, 2018

🤦 As I recall the maintainer wasn't exactly respectful of resolve-url-loader. That analysis I did at the time might interest you (with your 🥷 debug skilz).

Anyhow please let me know how the @next goes for you. Particularly if engine:rework works without regression it would be nice to hear that in #97.

Also feel free to keep commenting here too.

@bholloway
Copy link
Owner

bholloway commented Aug 29, 2018

Cool. Thanks for the comment in #97. I will shift back here to discuss in detail and we can return there if there is some conclusions...

🤔 Just thinking of bholloway/adjust-sourcemap-loader#3. Is it that there are these additional sources present? As I said there I don't use sourcesContent so I would need to check that code path.

Sounds like we will need a simple example project I can debug.

@rjgotten
Copy link
Author

rjgotten commented Aug 29, 2018

I've hooked back up my little 'source map spy' loader after resolve-url-loader and checked.
sourcesContent is present when the rework engine is used, but not when the postcss engine is used.

@bholloway
Copy link
Owner

Ah my fault, sorry. Look here.

Okay I will change that and run tests overnight (late here). Presumably I can do do an alpha.2 tomorrow and update dist-tag next.

@rjgotten
Copy link
Author

rjgotten commented Aug 29, 2018

@bholloway
Just found that. Looks like replacing it with "sourcesContent" in absSourceMap is what you want here.
I.e. if the incoming source map was already based on inlined sources; keep them. If not; don't bother making them.

Also; can I just congratulate you on the workaround of prepending the file:/ protocol to disk paths, to fix the mozilla/source-maps weirdness? Brilliant find.

@bholloway
Copy link
Owner

bholloway commented Aug 29, 2018

@rjgotten maybe I should just delete the highlighted line it should be true. Is that how you interpret options docs?

"By default, it is true. But if all previous maps do not contain sources content, PostCSS will also leave it out even if you do not set this option."

@bholloway
Copy link
Owner

@rjgotten can you pls monkey patch that line as sourcesContent: true and just make sure it is working for you in your local copy. I have the change in progress but immediate feedback is always good.

@rjgotten
Copy link
Author

@bholloway
It works with true locally. :)

@n-rodriguez
Copy link

n-rodriguez commented Aug 29, 2018

@bholloway : the same happens on Linux, got this error in Firefox console :

Impossible de récupérer la source d’origine : TypeError: NetworkError when attempting to fetch resource.
URL de la source : webpack:///node_modules/@fortawesome/fontawesome-free/scss/fontawesome.scss

Changing sourcesContent to true in local resolve-url-loader/lib/engine/postcss.js file solves the issue.

@bholloway
Copy link
Owner

bholloway commented Aug 29, 2018

Ah thanks @rjgotten @n-rodriguez.

So existing tests passed, I am moving forward with this change to the alpha. Will ping you when its released.

@n-rodriguez
Copy link

So existing tests passed, I am moving forward with this change to the alpha. Will ping you when its released.

Great :)

@bholloway
Copy link
Owner

Ping @rjgotten @n-rodriguez see comment.

Please let me know here if you have further feedback.

@bholloway
Copy link
Owner

Ping @rjgotten @n-rodriguez released 3.0.0. Please let me know if we can close this issue.

@n-rodriguez
Copy link

@bholloway : it's already in production :)

@bholloway
Copy link
Owner

Sounds good. Pleasure working with you on this. That said I'll add the footnote I've been using to close out the other existing issues 😉 ...

Please open a fresh issue for any ongoing problems and link to this one as needed.

I'm not opposed to reopening this issue but certainly lets talk with respect to the new version.

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

No branches or pull requests

3 participants