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 shadow function processing for async functions #3405

Merged
merged 7 commits into from
Mar 8, 2016

Conversation

loganfsmyth
Copy link
Member

This reverts and re-implements some of the work done in #3336 because while that fixed the rest params, it broke the ability to use arguments in your own code, since it would resolve to the arrow function rather than the outer function.

Also fixed https://phabricator.babeljs.io/T7108

Will squash if people feel like it, but I'd love to keep this split up to it's easier to follow. Not sure what the policy is specifically for non-work-in-progress commits.

* starting location path, the closest function will be used.
*
* Please Note, these flags are for private internal use only and should be avoided.
* Only "shadow" is a public property that other transforms may manipulate.
*/
Copy link
Member Author

Choose a reason for hiding this comment

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

Scoping shadowing is a bit....complicated right now to say the least. I didn't realize quite how hairy it was until I had to dive in for this. I ended up writing all this up for myself and figured I might as well include it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah... it's gotten pretty out of hand. It used to be a simple flag then it's use was overloaded (rightfully so since it's within scope, the API is just bad).

@hzoo
Copy link
Member

hzoo commented Mar 7, 2016

Snap! I don't think we really have a policy on commits - I don't really like the random/merge commits but none of it is really a big deal for me at least (we don't have a commit format atm either)

@hzoo hzoo added this to the 6.6.x milestone Mar 7, 2016
@hzoo hzoo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Mar 7, 2016
@codecov-io
Copy link

Current coverage is 87.62%

Merging #3405 into master will decrease coverage by -0.20% as of de21f2e

@@            master   #3405   diff @@
======================================
  Files          217     217       
  Stmts        15253   15253       
  Branches      3126    3128     +2
  Methods          0       0       
======================================
- Hit          13396   13365    -31
- Partial        594     624    +30
- Missed        1263    1264     +1

Review entire Coverage Diff as of de21f2e

Powered by Codecov. Updated on successful CI builds.

}

return false;
});

if (shadowFunction && fnPath.type === "Program" && shadowFunction.type !== "Program"){
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use the t.is helpers here, at least for consistency. We should seldom touch .type.

Copy link
Member

Choose a reason for hiding this comment

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

We should seldom touch .type

Can you expand on why that is? So it's more abstracted in case node types change? There've been cases where I've thought about going either way and I'd like to learn why the helpers are preferable.

@jmm
Copy link
Member

jmm commented Mar 8, 2016

I haven't looked at these commits, but in general:

  • I like logical commits. If something makes more sense in the history as separate commits I'm all for it and not in favor of squashing them arbitrarily just because they're in the same PR.
  • When tests are being added for something that was broken I like it when failing tests are committed and then fixes so you can checkout commits and see the tests fail then check out subsequent commits and see them pass.
  • When fixing mistakes in the PR commits (lint errors, typos, etc.), incorporating feedback, iterating after the PR is opened, I think it's usually good to rebase and squash those with the commit they would've originally made sense in (by the time it's merged anyway).

* Node's "shadow" props have the following behavior:
*
* - Boolean true will cause the function to shadow both "this" and "arguments".
* - {this: false} Shadows "this" but not "arguments".
Copy link
Member

Choose a reason for hiding this comment

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

did you mean "shadows arguments but not this"? Since it's {this: false}

loganfsmyth added a commit that referenced this pull request Mar 8, 2016
Fix shadow function processing for async functions
@loganfsmyth loganfsmyth merged commit bbc3401 into babel:master Mar 8, 2016
@loganfsmyth loganfsmyth deleted the shadowing-fixes branch March 8, 2016 06:38
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 7, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 7, 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 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.

6 participants