Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Remove ForAwaitStatement, add await flag to ForOfStatement #349

Merged
merged 2 commits into from
Feb 9, 2017

Conversation

aweary
Copy link
Contributor

@aweary aweary commented Feb 8, 2017

Q A
Bug fix? no
Breaking change? yes
New feature? no
Deprecations? no
Spec compliancy? yes
Tests added/pass? yes
Fixed tickets #348
License MIT

Removes the ForAwaitStatement node type and adds an await flag to the existing ForOfStatement node!

@codecov-io
Copy link

codecov-io commented Feb 8, 2017

Codecov Report

Merging #349 into 7.0 will not change coverage.

@@           Coverage Diff           @@
##              7.0     #349   +/-   ##
=======================================
  Coverage   97.65%   97.65%           
=======================================
  Files          19       19           
  Lines        3277     3277           
  Branches      867      867           
=======================================
  Hits         3200     3200           
  Misses         31       31           
  Partials       46       46
Impacted Files Coverage Δ
src/parser/statement.js 97.91% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0309a5b...3e9cb98. Read the comment docs.

@hzoo
Copy link
Member

hzoo commented Feb 8, 2017

Oops forgot to add the extra steps about changing to 7.0 branch (did it)

@hzoo hzoo changed the base branch from master to 7.0 February 8, 2017 20:45
@hzoo hzoo added this to the 7.0.0 milestone Feb 8, 2017
Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

haha I need to make the same change in estree

this.eatContextual("of");
type = "ForAwaitStatement";
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Should we set the node.await to false there? According to the changes in the spec I would expect a boolean.

Since undefined is falsy this isn't an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Do we do this elsewhere for other flags (false/true)? I think we might

Copy link
Member

Choose a reason for hiding this comment

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

I also think so. Don't have much context to answer you. Maybe @danez can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using Function as a reference, the generator and async flags are explicitly set to false when they don't apply, so it seems like it should do the same here.

@aweary
Copy link
Contributor Author

aweary commented Feb 9, 2017

@xtuc @hzoo I updated so the await flag should always be set on ForOfStatement nodes.

@xtuc
Copy link
Member

xtuc commented Feb 9, 2017

I updated the Babel 7 upgrade guide accordingly in babel/website@dd2586a

@danez
Copy link
Member

danez commented Feb 9, 2017

We need a follow-up story in babel-types, babel-generator and the transforms using ForAwaitStatement to account for this changes.

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.

LGTM, nice job 👍

@hzoo
Copy link
Member

hzoo commented Feb 9, 2017

why I wish babylon was in the monorepo again

@danez
Copy link
Member

danez commented Feb 9, 2017

I created a ticket babel/babel#5286

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants