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

Add support for ES iterators #1058

Merged
merged 1 commit into from
Jan 28, 2018

Conversation

maxnordlund
Copy link
Contributor

@maxnordlund maxnordlund commented Jan 26, 2018

Summary

Proposed change:

This requires the Symbol.iterator well known symbol and Array.from to coerce them into vanilla arrays.

This basically supersedes #760, see there for more info.

Checklist

I've completed the checklist below to ensure I didn't forget anything. This makes reviewing this PR as easy as possible for the maintainers. And it gets this change released as soon as possible.

@fdintino
Copy link
Collaborator

Just wanted to give you a heads-up that there is a fairly big refactor / cleanup in progress and under discussion at PR #1053. I intend to merge that into master sometime next week probably. Your best bet would be to try to open this as a patch onto the develop branch.

I'm not sure about this approach when it comes to generators. Someone using nunjucks that wanted to convert their generators to arrays could use Array.from themselves, whereas true support would actually use Generator.prototype.next() and only lazily evaluate results during iteration. That being said, I don't see an issue with using Array.from for Map and Set.

@fdintino fdintino requested review from jlongster and removed request for jlongster January 26, 2018 02:15
@maxnordlund maxnordlund force-pushed the add-support-for-iterator branch 2 times, most recently from fb66b82 to 421c8cc Compare January 26, 2018 17:03
@maxnordlund maxnordlund changed the base branch from master to develop January 26, 2018 17:04
@maxnordlund
Copy link
Contributor Author

maxnordlund commented Jan 26, 2018

Ok, I've updated this PR to point to develop and your totally right about the generators, so I've removed that from the docs.

Talking about generators, you really want to use for of here, but that requires transpilation if it's going to work in all browsers.

@codecov-io
Copy link

codecov-io commented Jan 26, 2018

Codecov Report

Merging #1058 into develop will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1058      +/-   ##
===========================================
+ Coverage    89.41%   89.44%   +0.02%     
===========================================
  Files           22       22              
  Lines         2966     2974       +8     
===========================================
+ Hits          2652     2660       +8     
  Misses         314      314
Impacted Files Coverage Δ
nunjucks/src/runtime.js 90.18% <100%> (+0.44%) ⬆️
nunjucks/src/compiler.js 95.5% <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 e6cc590...7086e2d. Read the comment docs.

@@ -475,6 +475,38 @@
},
'showing test\nshowing 1\nshowing 2\nshowing 3\n');
});
/* global Set */
if (typeof Set === 'function') {
it('should work with Set bultin', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like you have a typo here (and below) for 'builtin'.

Would it also be possible to rewrite this to use this.skip? e.g.:

it('should work with the Set builtin', function() {
  /* global Set */
 if (typeof Set === 'undefined') {
    this.skip();
  } else {
    equal(/* ... */);
  }
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

This requires the Symbol.iterator well known symbol and Array.from to
coerce them into vanilla arrays.
Copy link
Collaborator

@fdintino fdintino left a comment

Choose a reason for hiding this comment

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

This is great. Thanks for your contribution!

@fdintino fdintino merged commit 881e82d into mozilla:develop Jan 28, 2018
@maxnordlund
Copy link
Contributor Author

Always happy to help 😄

@maxnordlund maxnordlund deleted the add-support-for-iterator branch January 28, 2018 13:03
@@ -10,6 +10,8 @@ master (unreleased)

* Support objects created with Object.create(null). fixes [#468](https://github.com/mozilla/nunjucks/issues/468)

* Support ESNext iterators, using Array.from. Merge of
[#760](https://github.com/mozilla/nunjucks/pull/760)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean #1058? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, but I also want to mention that PR and I didn't understand what the format was. Would you mind making a PR that corrects this, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@maxnordlund maxnordlund Feb 4, 2018

Choose a reason for hiding this comment

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

Thank you

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.

4 participants