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: babel-plugin-proposal-function-bind: ts-ignore #14707

Conversation

coderaiser
Copy link
Contributor

@coderaiser coderaiser commented Jun 28, 2022

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

Get rid of ts-ignore in babel-plugin-proposal-function-bind. Added isMemberExpression check, since when there is no object, callee of BindExpression is Always MemberExpression, otherwise we see:

Binding should be performed on object property. (1:2)

for such code:

::console

So it should always used as MemberExpression:

::console.log

And produce:

BindExpression {
    callee: MemberExpression
}

In cases when we have object:

console::log

We have such signature:

BindExpression {
    callee: Expression,
    object: Expression
}

And both callee and object can be any Expression.

@babel-bot
Copy link
Collaborator

babel-bot commented Jun 28, 2022

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

@liuxingbaoyu
Copy link
Member

Could you add a test?

@liuxingbaoyu liuxingbaoyu added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Jun 29, 2022
@coderaiser
Copy link
Contributor Author

Could you add a test?

Test for what exactly? This is more like refactoring to fix TypeScript errors, this cases already tested in fixture directory.

@liuxingbaoyu
Copy link
Member

Ah, I mistakenly thought this was a bug fix.

@liuxingbaoyu liuxingbaoyu removed the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Jun 29, 2022
return bind.object;
}

t.assertMemberExpression(bind.callee);
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Does Babel parser ensures that bind.callee is always a member expression? If so we can remove the runtime assertion and just cast bind.callee as a member expression. Or we refine the AST types so that bind.callee must be a member expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, babel-parser ensures that, if no bind.object exists, this is expression like:

::console.log

And it cannot contain any other Expression or Babel parser throws:

Binding should be performed on object property. (1:2)

This check is needed for TypeScript, since it can infer types without it.
If you just remove the comment line:

// @ts-ignore Fixme: should check bind.callee type first

You will see the message:
image

So the purpose of PR is making TypeScript happy and removing a Fixme comment :). I agree that everything works good without this check either but, additional check makes code little bit more clear since function bind has two cases of work that supported by Babel (according to current implementation), as I wrote on the beginning.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good, I'm in favor of changing the definition of ast, as long as it doesn't compromise compatibility.

@coderaiser coderaiser requested a review from JLHwung July 4, 2022 18:04
@nicolo-ribaudo nicolo-ribaudo force-pushed the fix/babel-plugin-proposal-function-bind-ts-ignore branch from 9184945 to 88d11d1 Compare July 5, 2022 07:10
@nicolo-ribaudo nicolo-ribaudo merged commit f1c4ea3 into babel:main Jul 5, 2022
@coderaiser coderaiser deleted the fix/babel-plugin-proposal-function-bind-ts-ignore branch July 5, 2022 07:48
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 5, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants