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 order of optional argument reordering #8376

Merged
merged 3 commits into from
Jul 26, 2018
Merged

Conversation

Qix-
Copy link
Contributor

@Qix- Qix- commented Jul 24, 2018

Q                       A
Fixed Issues? None
Patch: Bug Fix? Yes.
Major: Breaking Change? No.
Minor: New Feature? No.
Tests Added + Pass? We'll see (minor enough to warrant waiting for CI)
Documentation PR Link N/A
Any Dependency Changes? No.
License MIT

Caught this looking into an unrelated issue with AST transformations. Previously, if the optional opts parameter wasn't passed, the intent was to move the function it held into the callback parameter and null out the opts param - but instead, it was nulling both.


I'd actually suggest that callback is checked, too. Otherwise, erroneous downstream code could pass two functions (one for the callback and one, erroneously, for the options), thereby swallowing what should be a thrown exception about incorrect types.

I can add that logic in if wanted.

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 24, 2018

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

@hzoo hzoo requested a review from loganfsmyth July 24, 2018 14:50
@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jul 24, 2018

👍 for the additional validation. Can you also add a test which shows how the old behavior was wrong?

Also, this bug probably needs to be fixed also in the other files in that folder.

@nicolo-ribaudo nicolo-ribaudo added pkg: core PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Jul 24, 2018
@loganfsmyth
Copy link
Member

loganfsmyth commented Jul 24, 2018

Woops, great catch. As @nicolo-ribaudo mentioned, we've got the same issue in

, so if you could fix those it'd be great.

@xtuc
Copy link
Member

xtuc commented Jul 25, 2018

Good catch!

After a quick search I found another one:

opts = undefined;

@hzoo
Copy link
Member

hzoo commented Jul 25, 2018

Added those ^, so we never added tests for async versions of those methods in #6780?

@hzoo
Copy link
Member

hzoo commented Jul 25, 2018

Can merge now as well to fix the issue, but yeah we should figure out the tests too

@hzoo hzoo merged commit db2a9fc into babel:master Jul 26, 2018
@Qix- Qix- deleted the fix-transform-ast branch July 26, 2018 17:10
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
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 pkg: core PR: Bug Fix 🐛 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

6 participants