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

Always use sourceFileName instead of sourceFilename. #10291

Closed
wants to merge 2 commits into from
Closed

Always use sourceFileName instead of sourceFilename. #10291

wants to merge 2 commits into from

Conversation

eventualbuddha
Copy link
Member

Q                       A
Fixed Issues? n/a
Patch: Bug Fix?
Major: Breaking Change? maybe
Minor: New Feature?
Tests Added + Pass? No, I'm not sure what changes are needed
Documentation PR Link
Any Dependency Changes?
License MIT

There seems to be some confusion around which of these two is correct within the codebase. This consolidates all instances to be sourceFileName since that one had more instances.

There seems to be some confusion around which of these two is correct within the codebase. This consolidates all instances to be `sourceFileName` since that one had more instances.
@hzoo hzoo requested a review from loganfsmyth August 1, 2019 23:42
Co-Authored-By: Henry Zhu <hi@henryzoo.com>
@@ -26,7 +26,7 @@ export default class Parser extends StatementParser {
this.inModule = this.options.sourceType === "module";
this.scope = new ScopeHandler(this.raise.bind(this), this.inModule);
this.plugins = pluginsMap(this.options.plugins);
this.filename = options.sourceFilename;
this.filename = options.sourceFilename || options.sourceFileName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the nullish coaleshing proposal is now stage 3, we could enable @babel/plugin-proposal-nullish-coalescing-operator and use options.sourceFilename ?? options.sourceFileName 😎

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been enabled on master, you just need to rebase.

Copy link
Member

@xtuc xtuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR.

I believe the documentation on the website already uses sourceFileName.

@eventualbuddha
Copy link
Member Author

I'm not sure what should be done about the test failures. Anyone else want to advise?

@nicolo-ribaudo
Copy link
Member

I'm not sure, maybe try rebasing it 🤔

@nicolo-ribaudo nicolo-ribaudo added the PR: Polish 💅 A type of pull request used for our changelog categories label Sep 6, 2019
@@ -1,5 +1,5 @@
type BabelParserOptions = {
sourceFilename?: string;
sourceFileName?: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a breaking change?

@eventualbuddha
Copy link
Member Author

Not sure what should be done to make this work, so I'm just going to close it. If someone else wants to pick it up, feel free.

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 24, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants