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 passing filename from @babel/core
to @babel/parser
#15911
base: main
Are you sure you want to change the base?
Fix passing filename from @babel/core
to @babel/parser
#15911
Conversation
75dd091
to
3fd6f52
Compare
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/55322/ |
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/55316/ |
packages/babel-core/test/api.js
Outdated
}); | ||
|
||
expect(result.map.sources).toEqual(["path.js", "file/path.js"]); |
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 believe the old one here was wrong, but:
- is the new one correct?
- having both is definitely wrong
95f0ba9
to
82cc0e0
Compare
}); | ||
|
||
expect(result.map.sources).toEqual(["file/path.js"]); |
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'm not sure if we should consider this PR as breaking because of this change, or as a bugfix. I'm not even sure if this is correct, but it's probably the best we can do given that we do not know the final location of the source map on disk.
@@ -0,0 +1,339 @@ | |||
{ |
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 file is a copy of output.json
, but every loc has the filename
property.
@@ -30,7 +39,10 @@ export default function normalizeOptions(config: ResolvedConfig): {} { | |||
sourceType: | |||
path.extname(filenameRelative) === ".mjs" ? "module" : sourceType, | |||
|
|||
sourceFileName: 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 plugin babel-plugin-bundled-import-meta
is relying on this option to get the complete input filename,
https://github.com/cfware/babel-plugin-bundled-import-meta/blob/master/index.js#L67
We reverted the change in #13732, we will probably have to defer this PR to Babel 8.
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.
We could keep passing sourceFileName
to avoid breaking plugins that rely on it -- given that it's ignored by @babel/parser
, there is no harm in doing so.
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.
If we start to pass both base filename as sourceFilename
for source map correctness, and complete filename as sourceFileName
for legacy compatibility, that would be indeed confusing. The parser docs should then clarify that sourceFileName
is deprecated and we offer a transition strategy, maybe a new property in the File
class offered by @babel/core
?
Fixes #1, Fixes #2
This is blocking #9509. All the source map tests are now broken, and I have to figure out what is the correct behavior.