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

fix: non breaking align options naming #13532

Merged
merged 2 commits into from Aug 19, 2021

Conversation

@fedeci
Copy link
Member

@fedeci fedeci commented Jul 6, 2021

Q                       A
Fixed Issues? Fixes #13170
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

@fedeci
Copy link
Member Author

@fedeci fedeci commented Jul 6, 2021

Let's see if it breaks e2e🤞

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Jul 6, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/47767/

@codesandbox
Copy link

@codesandbox codesandbox bot commented Jul 6, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8917e57:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@fedeci
Copy link
Member Author

@fedeci fedeci commented Jul 6, 2021

May we consider it a new feature because previously this was "shadowed" and not accessible?

@fedeci fedeci marked this pull request as ready for review Jul 6, 2021
@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jul 6, 2021

Yeah I'd consider this a new feature.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Can you also add a test that directly passes the sourceFileName option?
And also a test for the unknown special case (why do we need that special case?)

@fedeci fedeci force-pushed the align-options-naming-babel-7 branch 2 times, most recently from 4d6046f to 0d172f3 Jul 21, 2021
cwd: fixture(),
});
expect(JSON.parse(JSON.stringify(result))).toEqual(output);
});

it("should parse using passed in configuration", function () {
const input = fs.readFileSync(fixture("input.js"), "utf8");
const output = require(fixture("output.json"));
const input = fs.readFileSync(
Copy link
Member Author

@fedeci fedeci Jul 21, 2021

@nicolo-ribaudo this tests the no-"unknown" handling because no filename, filenameRelative or sourceFileName is passed.
We need that special case handling because sourceFileName defaults to filenameRelative which defaults to "filename" (if provided) or to "unknown".

@fedeci fedeci force-pushed the align-options-naming-babel-7 branch from 0d172f3 to 8917e57 Aug 1, 2021
@nicolo-ribaudo nicolo-ribaudo requested a review from hzoo Aug 11, 2021
hzoo
hzoo approved these changes Aug 19, 2021
@JLHwung JLHwung merged commit 51a0caa into babel:main Aug 19, 2021
25 of 27 checks passed
@fedeci fedeci deleted the align-options-naming-babel-7 branch Aug 23, 2021
@LarsDenBakker
Copy link

@LarsDenBakker LarsDenBakker commented Sep 3, 2021

This change breaks one of my babel plugins that relies on the parserOverride hook. The value of sourceFileName has now changed to be the file basename instead of the path to the file.

@fedeci
Copy link
Member Author

@fedeci fedeci commented Sep 3, 2021

That should be the right behaviour but yeah, since it breaks let's leave it entirely for babel-8.

@LarsDenBakker
Copy link

@LarsDenBakker LarsDenBakker commented Sep 3, 2021

@fedeci thanks! Shouldn't we add filename as well to parserOpts then? Otherwise in the parserOverride hook there is no way to know where the actual file is located.

I have a babel plugin that transforms HTML Imports to es modules, and it transforms the HTML to valid JS in this hook. I need to know the folder of the file for resolving some references inside the HTML.

@fedeci
Copy link
Member Author

@fedeci fedeci commented Sep 3, 2021

Probably yes, I'll update the other PR in these days!

@dlockhart
Copy link

@dlockhart dlockhart commented Sep 3, 2021

Noticed this change is breaking this plugin here, which is probably the same/similar thing @LarsDenBakker is referring to.

@DylanPiercey
Copy link

@DylanPiercey DylanPiercey commented Sep 3, 2021

If we're already considering this a breaking change, would it not make more sense to standardize all uses of sourcefilename to be the same casing? Currently the parserOpts is the only one that expects the lowercase n.

DylanPiercey added a commit to marko-js/marko that referenced this issue Sep 3, 2021
babel/babel#13532
Babel released a patch that included renaming a config property.
They are planning to reverse this change. This patch works with
both the previous and current versions of babel.
@fedeci
Copy link
Member Author

@fedeci fedeci commented Sep 3, 2021

@DylanPiercey this was not to be considered as a breaking change initially because of my fault. It will be reverted in the next patch version. The "big breaking change" is already scheduled for babel 8.

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Sep 4, 2021

I'm releasing v7.15.5 to revert this.

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

Successfully merging this pull request may close these issues.

8 participants