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

[Breaking] 2018/2019: GetIterator: Add hint parameter #67

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Nov 4, 2019

Addresses part of #66.

BREAKING CHANGE: This is backwards incompatible, as the hint parameter is added between the obj and method parameters.


review?(@ljharb)

BREAKING CHANGE: This is backwards incompatible, as the `hint` parameter is added between the `obj` and `method` parameters.
es2018.js Outdated
Comment on lines 192 to 200
// TODO: This should return an IteratorRecord
/*
var nextMethod = this.GetV(iterator, 'next');
return {
'[[Iterator]]': iterator,
'[[NextMethod]]': nextMethod,
'[[Done]]': false
};
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also a breaking change, which should probably be made as part of the v2.0 release, which should be where this gets first released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m doing this in #68.

@codecov
Copy link

codecov bot commented Nov 7, 2019

Codecov Report

Merging #67 into master will decrease coverage by 0.52%.
The diff coverage is 77.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
- Coverage   90.82%   90.29%   -0.53%     
==========================================
  Files         828      828              
  Lines       11541    11567      +26     
  Branches     2652     2662      +10     
==========================================
- Hits        10482    10445      -37     
- Misses       1059     1122      +63     
Impacted Files Coverage Δ
2018/GetIterator.js 82.75% <75.00%> (-11.00%) ⬇️
2019/GetIterator.js 82.75% <75.00%> (-11.00%) ⬇️
2018/IterableToList.js 100.00% <100.00%> (ø)
2019/IterableToList.js 100.00% <100.00%> (ø)
2020/GetIterator.js 82.75% <100.00%> (ø)
helpers/getIteratorMethod.js 34.61% <0.00%> (-65.39%) ⬇️
helpers/setProto.js 44.44% <0.00%> (-44.45%) ⬇️
2018/PromiseResolve.js 83.33% <0.00%> (-16.67%) ⬇️
2019/PromiseResolve.js 83.33% <0.00%> (-16.67%) ⬇️
2020/PromiseResolve.js 83.33% <0.00%> (-16.67%) ⬇️
... and 13 more

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 ddbba60...b967be4. Read the comment docs.

@ExE-Boss ExE-Boss mentioned this pull request Nov 7, 2019
3 tasks
@ExE-Boss ExE-Boss changed the title feat(ES2018): Add hint parameter to GetIterator [Breaking] 2018+: GetIterator: Add hint parameter May 16, 2020
@ExE-Boss
Copy link
Contributor Author

@ljharb I’ve rebased this, but some builds didn’t run due to GitHub rate‑limiting Travis CI.

ljharb added a commit that referenced this pull request Oct 1, 2020
See #67

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@ljharb
Copy link
Owner

ljharb commented Oct 1, 2020

I've pulled these changes into ES2020 in ddbba60, and rebased this PR, so now it's only breaking for ES2018 and ES2019 - and ES2020+ will have the proper signature, and adding async iterator support will be semver-minor.

@ljharb ljharb changed the title [Breaking] 2018+: GetIterator: Add hint parameter [Breaking] 2018/2019: GetIterator: Add hint parameter Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants