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.

Sign up for GitHub

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

Jump to bottom

Porting babel-plugin-transform-for-of-as-array into transform-for-of as option #6914

Merged
merged 5 commits into from
Nov 30, 2017
Merged

Porting babel-plugin-transform-for-of-as-array into transform-for-of as option #6914

merged 5 commits into from
Nov 30, 2017

Conversation

rajasekarm
Copy link
Member

Q                       A
Fixed Issues? #6816
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

@rajasekarm rajasekarm mentioned this pull request Nov 27, 2017
Closed
@rajasekarm
Copy link
Member Author

rajasekarm commented Nov 27, 2017 •
edited
Loading

Usage:-

{
  "plugins": [["transform-for-of", {
    "assumeArray": true
    }]
  ]
}

@babel-bot
Copy link
Collaborator

babel-bot commented Nov 27, 2017 •
edited
Loading

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

Andarist
Andarist reviewed Nov 27, 2017
View reviewed changes
packages/babel-plugin-transform-for-of/src/index.js Outdated
@@ -1,7 +1,7 @@
import { template, types as t } from "@babel/core";

export default function(api, options) {
const { loose } = options;
const { loose, forOfAsArray: isForOfAsArray } = options;
Copy link
Member

Choose a reason for hiding this comment

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

would prefer a little bit more descriptive name - useForAsArray or something and that name could be used both as the internal flag and the public plugin option

Copy link
Member

Choose a reason for hiding this comment

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

What about just assmeArray? After all, for of is already specified in the plugin name.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo assumeArray?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, assumeArray sounds like a great name for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I will update the PR.

@Andarist Andarist added PR: New Feature 🚀 A type of pull request used for our changelog categories PR: WIP labels Nov 27, 2017
@nicolo-ribaudo
Copy link
Member

  1. Since the loose and assumeArray options are not compatible, I think we should throw an error when both are true.
  2. If assumeArray if true, we can avoid defining the buildForOfArray, buildForOfLoose and buildForOf templates by returing earlier.

existentialism
existentialism reviewed Nov 28, 2017
View reviewed changes
packages/babel-plugin-transform-for-of/src/index.js Outdated

if (loose === true && assumeArray === true) {
throw new Error(
`Cannot use loose and assumeArray options together in babel-plugin-transform-for-of`,
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

The loose and assumeArray options cannot be used together in @babel/plugin-transform-for-of

existentialism
existentialism approved these changes Nov 29, 2017
View reviewed changes
Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

Can we document the assumeArray option in the README?

hzoo
hzoo reviewed Nov 29, 2017
View reviewed changes
packages/babel-plugin-transform-for-of/src/index.js Outdated
@@ -107,49 +161,6 @@ export default function(api, options) {
}
}

return {
Copy link
Member

Choose a reason for hiding this comment

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

none of the code here is any different right? (just saying this part of the diff isn't necessary as clarification)

hzoo
hzoo approved these changes Nov 29, 2017
View reviewed changes
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.

looks good (if you are cool with changing the other code back for a simplier diff that would be nice), yeah we should have a readme change to document

@hzoo hzoo requested a review from jridgewell November 29, 2017 15:20
@hzoo
Copy link
Member

hzoo commented Nov 30, 2017

We can test this by using it lol, but can be another PR

babel/packages/babylon/package.json

Line 27 in d8bbaaa

"babel-plugin-transform-for-of-as-array": "1.0.4",

@hzoo hzoo merged commit a992d06 into babel:master Nov 30, 2017
@hzoo hzoo mentioned this pull request Nov 30, 2017
Merged
@existentialism existentialism mentioned this pull request Dec 13, 2017
Closed
@rajasekarm rajasekarm deleted the f-6816 branch December 16, 2017 03:32
@jridgewell jridgewell mentioned this pull request Feb 1, 2018
Closed
@jdalton jdalton mentioned this pull request Feb 1, 2018
Closed
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@existentialism existentialism existentialism approved these changes

@hzoo hzoo hzoo approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

@Andarist Andarist Andarist approved these changes

@jridgewell jridgewell Awaiting requested review from jridgewell

Assignees
No one assigned
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@rajasekarm @babel-bot @nicolo-ribaudo @hzoo @existentialism @Andarist