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.

Sign up for GitHub

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

Jump to bottom

Fix passing filename from @babel/core to @babel/parser #15911

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+374 −12
Open

Fix passing filename from @babel/core to @babel/parser #15911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3fd6f52
Fix passing filename from `@babel/core` to `@babel/parser`
nicolo-ribaudo Aug 31, 2023
6777110
Use relative filename
nicolo-ribaudo Aug 31, 2023
8b61443
Update snapshot
nicolo-ribaudo Aug 31, 2023
4ac75e8
Update test (WRONG)
nicolo-ribaudo Aug 31, 2023
82cc0e0
Pass relative filename to generator also
nicolo-ribaudo Aug 31, 2023
a5e38f9
Fix behavior with sourceRoot
nicolo-ribaudo Aug 31, 2023
0505343
Fix mappings generation
nicolo-ribaudo Aug 31, 2023
18fb9a8
Add TODO comment
nicolo-ribaudo Aug 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 26 additions & 3 deletions packages/babel-core/src/transformation/normalize-opts.ts
View file
Open in desktop
Expand Up @@ -2,6 +2,13 @@ import path from "path";
import type { ResolvedConfig } from "../config/index.ts";

export default function normalizeOptions(config: ResolvedConfig): {} {
// TODO: Everything in this function is basically typed as `any`. Improve it.

// TODO(@nicolo-ribaudo): Currently, soure map's `sources` is generated taking
// into account both the options passed to the parser and the options passed
// to the generator. If they disagree, both are included. Clean this up, so
// that there is a single source of thruth.

const {
filename,
cwd,
Expand All @@ -15,7 +22,9 @@ export default function normalizeOptions(config: ResolvedConfig): {} {
? undefined
: config.options.moduleRoot,

sourceFileName = path.basename(filenameRelative),
sourceFileName = filenameRelative === "unknown"
? undefined
: filenameRelative,

comments = true,
compact = "auto",
Expand All @@ -30,7 +39,10 @@ export default function normalizeOptions(config: ResolvedConfig): {} {
sourceType:
path.extname(filenameRelative) === ".mjs" ? "module" : sourceType,

sourceFileName: filename,
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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?

// TODO: @babel/parser uses sourceFilename, while @babel/generator and
// @babel/core use sourceFileName. Eventualy align them.
// https://github.com/babel/babel/pull/13518
sourceFilename: sourceFileName,
plugins: [],
...opts.parserOpts,
},
Expand All @@ -51,7 +63,18 @@ export default function normalizeOptions(config: ResolvedConfig): {} {
sourceMaps,

sourceRoot,
sourceFileName,
sourceFileName:
// If there is no filename, we use `"unknown"` in the source map
// `sources` array as a fallback. Due to how @babel/generator works,
// if we passed `undefined` there would be no generated mappings.
// Additionally, `undefined` isn't JSON-serializable.
sourceFileName == null
? "unknown"
: sourceRoot == null
? sourceFileName
: // @babel/generator will prepend sourceFileName with sourceRoot,
// so we need to remove it here.
path.relative(sourceRoot, sourceFileName),
...opts.generatorOpts,
},
};
Expand Down
8 changes: 4 additions & 4 deletions packages/babel-core/test/api.js
View file
Open in desktop
Expand Up @@ -648,14 +648,14 @@ describe("api", function () {
});
});

it("default source map filename", function () {
return transformAsync("var a = 10;", {
it("default source map filename", async function () {
const result = await transformAsync("var a = 10;", {
cwd: "/some/absolute",
filename: "/some/absolute/file/path.js",
sourceMaps: true,
}).then(function (result) {
expect(result.map.sources).toEqual(["path.js"]);
});

expect(result.map.sources).toEqual(["file/path.js"]);
Copy link
Member Author

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.

});

it("code option false", function () {
Expand Down