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

Implement iterableIsArray and arrayLikeIsIterable assumptions #12489

Merged
merged 3 commits into from
Dec 14, 2020
Merged

Implement iterableIsArray and arrayLikeIsIterable assumptions #12489

merged 3 commits into from
Dec 14, 2020

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Dec 11, 2020 •
edited by gitpod-io bot

Q                       A
Major: Breaking Change? Yes
Tests Added + Pass? Yes
License MIT

Main PR: #12219
RFC: babel/rfcs#5

Picking the correct defaults was hard in this PR, so please check if what I did makes sense.

@nicolo-ribaudo nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories area: assumptions labels Dec 11, 2020
@babel-bot
Copy link
Collaborator

babel-bot commented Dec 11, 2020 •
edited

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

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1e64d8f:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Dec 11, 2020
Merged
24 tasks
@nicolo-ribaudo
Implement iterableIsArray
0aa8803 
- transform-destructuring
- transform-for-of
- transform-spread
@nicolo-ribaudo
Implement arrayLikeIsIterable
c333a86 
- transform-destructuring
- transform-for-of
- transform-spread
@nicolo-ribaudo
Update options defaults
a029176
JLHwung
JLHwung approved these changes Dec 11, 2020
View reviewed changes
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Are we going to rename allowArrayLike to arrayLikeIsIterable in Babel 8?

@nicolo-ribaudo
Copy link
Member Author

If we keep it yes; I'm not sure if it's it worth to keep it around given the new assumptions.arrayLikeIsIterable.

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Dec 12, 2020
Merged
@nicolo-ribaudo nicolo-ribaudo merged commit a275e00 into babel:feat-7.13.0/babel-core-features Dec 14, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the assumption/array branch December 14, 2020 22:38
jridgewell
jridgewell reviewed Dec 15, 2020
View reviewed changes
packages/babel-plugin-transform-destructuring/src/index.js
const arrayOnlySpread = loose;
const iterableIsArray = api.assumption("iterableIsArray") ?? options.loose;
const arrayLikeIsIterable =
options.allowArrayLike ?? api.assumption("arrayLikeIsIterable");
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this backwards?

jridgewell
jridgewell reviewed Dec 15, 2020
View reviewed changes
packages/babel-plugin-transform-spread/src/index.js
const { loose, allowArrayLike } = options;
const iterableIsArray = api.assumption("iterableIsArray") ?? options.loose;
const arrayLikeIsIterable =
options.allowArrayLike ?? api.assumption("arrayLikeIsIterable");
Copy link
Member

Choose a reason for hiding this comment

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

Ditto backwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for swapping the order here is that, while loose is generic (and thus it's good to keep it as a fallback value), this option was explicitly designed for this assumption and it has a quite specific name. Thus I followed the "locality principle": since they have the same specificity, the option closer to the plugin has the precedence.

I wasn't 100% sure of doing this, so I'm open to change it to match loose's behavior if no one else thinks that options.allowArrayLike should take precedence.

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to me that when options.allowArrayLike is specified, that it should take precedence over api.assumption("arrayLikeIsIterable").

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jan 12, 2021
@nicolo-ribaudo
Implement iterableIsArray and arrayLikeIsIterable (babel#12489)
5ab11fc
nicolo-ribaudo added a commit that referenced this pull request Jan 24, 2021
@nicolo-ribaudo
Implement iterableIsArray and arrayLikeIsIterable (#12489)
c4ba4e5
nicolo-ribaudo added a commit that referenced this pull request Jan 24, 2021
@nicolo-ribaudo
Implement iterableIsArray and arrayLikeIsIterable (#12489)
df1352c
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Feb 4, 2021
@nicolo-ribaudo
Implement iterableIsArray and arrayLikeIsIterable (babel#12489)
799ed2d
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Feb 10, 2021
@nicolo-ribaudo
Implement iterableIsArray and arrayLikeIsIterable (babel#12489)
789ba82
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Feb 11, 2021
@nicolo-ribaudo
Implement iterableIsArray and arrayLikeIsIterable (babel#12489)
b2b9d75
nicolo-ribaudo added a commit that referenced this pull request Feb 16, 2021
@nicolo-ribaudo @jridgewell @JLHwung
Implement assumptions defined in the babel/rfcs#5 RFC
90a674b 
- `mutableTemplateObject` and `ignoreToPrimitiveHint` (#12408)
- `setClassMethods` (#12407)
- `setComputedProperties` (#12490)
- `ignoreFunctionLength` (#12491)
- `noDocumentAll` (#12481)
- `iterableIsArray` and `arrayLikeIsIterable` (#12489)
- `pureGetters` (#12504)
- `skipForOfIteratorClosing` (#12496)
- `objectRestNoSymbols`, `setSpreadProperties` and `pureGetters` (#12505)
- `noNewArrows` (#12613, #12793)
- `setPublicClassFields` and `privateFieldsAsProperties` (#12497)
- `constantReexports` and `enumerableModuleMeta` (#12618)
- `constantSuper`, `superIsCallableConstructor` and `noClassCalls` (#12726)

Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
Co-authored-by: Huáng Jùnliàng <JLHwung@users.noreply.github.com>
nicolo-ribaudo added a commit that referenced this pull request Feb 16, 2021
@nicolo-ribaudo @jridgewell @JLHwung
Implement assumptions defined in the babel/rfcs#5 RFC
551b5f0 
- `mutableTemplateObject` and `ignoreToPrimitiveHint` (#12408)
- `setClassMethods` (#12407)
- `setComputedProperties` (#12490)
- `ignoreFunctionLength` (#12491)
- `noDocumentAll` (#12481)
- `iterableIsArray` and `arrayLikeIsIterable` (#12489)
- `pureGetters` (#12504)
- `skipForOfIteratorClosing` (#12496)
- `objectRestNoSymbols`, `setSpreadProperties` and `pureGetters` (#12505)
- `noNewArrows` (#12613, #12793)
- `setPublicClassFields` and `privateFieldsAsProperties` (#12497)
- `constantReexports` and `enumerableModuleMeta` (#12618)
- `constantSuper`, `superIsCallableConstructor` and `noClassCalls` (#12726)

Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
Co-authored-by: Huáng Jùnliàng <JLHwung@users.noreply.github.com>
nicolo-ribaudo added a commit that referenced this pull request Feb 21, 2021
@nicolo-ribaudo @jridgewell @JLHwung
Implement assumptions defined in the babel/rfcs#5 RFC
6ef7b51 
- `mutableTemplateObject` and `ignoreToPrimitiveHint` (#12408)
- `setClassMethods` (#12407)
- `setComputedProperties` (#12490)
- `ignoreFunctionLength` (#12491)
- `noDocumentAll` (#12481)
- `iterableIsArray` and `arrayLikeIsIterable` (#12489)
- `pureGetters` (#12504)
- `skipForOfIteratorClosing` (#12496)
- `objectRestNoSymbols`, `setSpreadProperties` and `pureGetters` (#12505)
- `noNewArrows` (#12613, #12793)
- `setPublicClassFields` and `privateFieldsAsProperties` (#12497)
- `constantReexports` and `enumerableModuleMeta` (#12618)
- `constantSuper`, `superIsCallableConstructor` and `noClassCalls` (#12726)

Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
Co-authored-by: Huáng Jùnliàng <JLHwung@users.noreply.github.com>
@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 Mar 17, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jridgewell jridgewell jridgewell left review comments

@ExE-Boss ExE-Boss ExE-Boss left review comments

@hzoo hzoo hzoo approved these changes

@JLHwung JLHwung JLHwung approved these changes

Assignees
No one assigned
Labels
area: assumptions 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
@nicolo-ribaudo @babel-bot @jridgewell @hzoo @JLHwung @ExE-Boss