-
Notifications
You must be signed in to change notification settings - Fork 263
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 source maps #551
Fix source maps #551
Conversation
src/_utils.js
Outdated
@@ -591,7 +591,8 @@ export const processCss = (stylesInfo, options) => { | |||
|
|||
if (useSourceMaps) { | |||
const generator = makeSourceMapGenerator(fileInfo.file) | |||
const filename = fileInfo.sourceFileName | |||
const filename = fileInfo.sourceFileName || fileInfo.filename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bug was here, these are not defined when not using the CLI I guess. Chances are that source maps still work when the plugin is used with babel loader or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started looking into this too and actually just opened a pr not seeing yours 🤦♂️
On my branch I used file.opts.generatorOpts.sourceFileName
since it seems to match with the value we were expecting before. I closed it after seeing yours, but I just re-opened it so you can take a look.
src/_utils.js
Outdated
// According to https://babeljs.io/docs/en/options#source-map-options | ||
// filenameRelative = path.relative(file.opts.cwd, file.opts.filename) | ||
// sourceFileName = path.basename(filenameRelative) | ||
(fileInfo.filename && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ijjk what do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems more stable than trying to follow where it's at in the options. Doesn't path.basename
just return the last part of the path so the path.relative
isn't necessary inside of the path.basename
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 indeed.
Fixes #547