Add support for all iterable children, not just Arrays

[neeharv]

This will fix https://github.com/developit/preact-compat/issues/262

This is still WIP, as this fix obviously changes behaviour wrt the children being added to the stack in reverse order.

I wanted to have a discussion on the best way to push the children on to the stack, considering there is no reverse iterable protocol (yet). I could push them all to an array and then call .reverse() on it, but I'm unsure of the perf implications if there are a large number of children.

[developit]

Ah! I should have looked before I responded in 282! I'll take a look at this as soon as I can, happy to see the PR!

[developit]

Initial thought: it would be good to find a way around relying on Symbol here.

[NekR]

Also, Babel's for..of generated a lot of code because it relies on Symbol and then fallbacks to Array. So check for Symbol might not be needed there if you are going that way.

[developit]

Ah yeah I want to avoid anything transpiled/polyfilled entirely if we can. Perhaps convert iterables to Arrays? We only ever care about the values.

[NekR]

TBH I just don't see why support iterables right now. I mean, they aren't everywhere supported natively so typically you will need to convert, say HTMLCollection to Array anyway in your code.

The only possible way here when it might be needed is when developers provide fully supported environment on their own, i.e. run polyfill before preact. Sounds like an edge case to me (though, of course, I can wrong).

[neeharv]

@NekR well, my personal use case stems from developit/preact-compat#262. Since React allows iterable structures as children, it would be reasonable to assume that other developers coming to Preact would also expect similar behaviour. Additionally, not restricting iterable children to Arrays opens up use cases for libs like Immutable or newer data structures like Map and Set, heck even generators to be used. Constraining userland code to convert these data structures to Arrays may be impractical and a big loss for usability IMO.

[neeharv]

@NekR @developit So if I understand correctly, there are 3 choices in front of us

1. Polyfill for...of and Symbol, and go with the approach in this PR
   • Pros - Idiomatic way to do it (IMO), plus easier to grok
   • Cons - Increase code size, perf hit (?), need to reverse iteration order to preserve current stack order semantics
2. Convert iterables to an array using Array.from
   • Pros - Keeps existing way of iterating and pushing to stack the same
   • Cons - Needs polyfill, unsure of perf hit
3. Don't support iterables as children, and add a nice-ish error when non array-like iterables are passed down, letting users know they need to convert to Arrays before calling it down
   • Pros - No changes
   • Cons - Not very intuitive when coming over from React, having this allows more idiomatic patterns of constructing children, doesn't allow use with black box data structure libraries like Immutable

[NekR]

First 2 ways do 2 iterations (reverse + iterate; from + iterate). If we are going to support this, then we should write iterater on our own and avoid traspiled code and double iterations.

On Dec 29, 2016 1:05 PM, "Neehar Venugopal" [email protected] wrote:

@NekR https://github.com/NekR @developit https://github.com/developit So if I understand correctly, there are 3 choices in front of us

1. Polyfill for...of and Symbol, and go with the approach in this PR
   • Pros - Idiomatic way to do it (IMO), plus easier to grok
   • Cons - Increase code size, perf hit (?), need to reverse iteration order to preserve current stack order semantics
2. Convert iterables to an array using Array.from https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/from
   • Pros - Keeps existing way of iterating and pushing to stack the same
   • Cons - Needs polyfill, unsure of perf hit
3. Don't support iterables as children, and add a nice-ish error when non array-like iterables are passed down, letting users know they need to convert to Arrays before calling it down
   • Pros - No changes
   • Cons - Not very intuitive when coming over from React, having this allows more idiomatic patterns of constructing children, doesn't allow use with black box data structure libraries like Immutable

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/developit/preact/pull/476#issuecomment-269607791, or mute the thread https://github.com/notifications/unsubscribe-auth/ABIlkVRJwOPOa4cspfBUVK7vZJHzaDG0ks5rM4XegaJpZM4LW0Gm .

[neeharv]

@NekR unfortunately even if we write our own code to consume the iterator, the iterator protocol is well defined and implemented across the board. And that protocol does not define a reverse iterator ( I believe there is a strawman proposal here). So if we need to support this, then there is no other way than to iterate + reverse.

[robertknight]

Not very intuitive when coming over from React, having this allows more idiomatic patterns of constructing children, doesn't allow use with black box data structure libraries like Immutable

Hmm. If you are using Preact in order to reduce the script size of your application, are you likely to be using a library like Immutable which is much larger than Preact itself as opposed to something like seamless-immutable?

I'll throw a variant of option (2) (Convert iterables to an array using Array.from) into the mix. Convert iterables to an array using Array.from, but only if the environment already provides Array.from (either natively or via a polyfill).

Pros: Less code in applications that don't need the feature or already have the polyfill Cons: Consumers of the library may depend on this feature without realizing that a polyfill is required

[developit]

Ahh I like that option @robertknight. If Array.from provides a tiny way to create an Array from an Iterable, it seems like a reasonably direct solution. For performance, maybe we recommend a well-suited polyfill or allow defining an iterator-to-array function on options?

[neeharv]

@developit @robertknight That seems like a reasonable solution. The only caveat I have is that if we do something along the lines of detecting if Array.prototype.from exists, and only then allow this behaviour, consumers might use it without realising that they need to polyfill it.

Should we then do the following -

1. If child is an Array, continue existing behaviour
2. If child is an iterable, then convert it to an array using Array.prototype.from, but also notify the user in dev mode (?) that if you're using iterables, you need to add an Array.from polyfill (I like the idea of a userland polyfill more than passing down a iterable->array function down as options, since Array.from is the semantic way of converting iterable->array)
3. If an iterable is used, and Array.from isn't available, warn the user and discard those children

[neeharv]

Hmm. If you are using Preact in order to reduce the script size of your application, are you likely to >be using a library like Immutable which is much larger than Preact itself as opposed to something >like seamless-immutable @robertknight Immutable is just an example of a userland data structure lib. With the introduction of the iterable protocol, there exists an interoperable way to use older data structures (Array), ES2015 ones (Map, Set) as well as third party libs. Going forward, I'd expect lots of people to use whatever data structure fits there need, knowing that libraries have a standardised way of iterating over them.

As for my personal use case with preact, I'm switching to it not just because of size, but because it has the best bits of React without any of the cruft. I don't need a synthetic event system, I don't need propTypes validation, so why pay for that cost. With preact + immutable, my js is still well under 100kb, and with SSR that feels blazing fast on 3G!

[NekR]

@neeharv unfortunately there is no dev mode in Preact yet.

Just some code to consider:

[developit]

Hmm. If is there a way to manually pull an Iterable into an array? We only care about values. I have done the reverse (feign Iterable API using an Array to avoid relying on Iterables themselves).

[robertknight]

Hmm. If is there a way to manually pull an Iterable into an array?

Using Babel's transpilation of for ... of to handle the iteration protocol:

function from(iterable) {
  // TODO: Check for `Symbol` and `iterable[Symbol.iterator]` to test whether this
  // object is actually an iterable.
  let ary = [];
  for (let item of iterable) {
    ary.push(item);
  }
  return ary;
}

[neeharv]

Using Babel's transpilation of for ... of to handle the iteration protocol:

This is exactly what I have done in this PR 😄 @robertknight

In addition, since true iterables can not be polyfilled, this implementation does not support generic iterables as defined in the 6th edition of ECMA-262.

From here

@developit even if one were to use a "dumb" polyfill for Array.from, we can't support generic iterables.

The only way to do that would be to use Symbol.iterator. I see no way around adding support for iterables without some sort pf polyfill at the very least. Even to detect that a child passed in is an iterable would need a polyfill for Symbol.iterator

I could do some benchmarking in terms of build size if Symbol and Array.from are polyfilled. Let me know your thoughts on adding this to preact. Personally I feel it'd be a hugely limiting not to have it going forward, but I understand preact prioritises size and speed!

[NekR]

@developit can we just have option.toArray hook of something like that? Then those who need iterables work could just hook in that functionality. We can even publish it to npm as separate package, I guess.

[robertknight]

This is exactly what I have done in this PR 😄 @robertknight

Ah, sorry. I wasn't paying enough attention 🤦‍♂️

Even to detect that a child passed in is an iterable would need a polyfill for Symbol.iterator

If the child passed in is an iterable presumably that means that Symbol.iterator must already have been defined or polyfilled in order to implement the interface. So Preact can just check for Symbol being an object and having an iterator property - it doesn't even need to provide the implementation itself.

[neeharv]

If the child passed in is an iterable presumably that means that Symbol.iterator must already have been defined or polyfilled in order to implement the interface. So Preact can just check for Symbol being an object and having an iterator property - it doesn't even need to provide the implementation itself.

That makes sense! I'm bummed out I didn't think of that. I've implemented a simple iterable to array util function, plus updated tests to use both built in Set and List from immutable.js to also have a test against #282

This PR should be ready to be merged in now @developit.

P.S @robertknight typeof Symbol === 'function' 🙃

[developit]

Wow, awesome work guys.

[developit]

@robertknight figured that'd throw, I should have said something. Might actually be able smaller to check if (typeof ATTR_KEY!='string') - ATTR_KEY is a Symbol when Symbol is available. Or maybe combine the checks into one if().

FWIW I need to check a few things relating to size and perf before we merge this. Ideally I'd like to do that prior to this hitting master so it doesn't hold back 7.2, which is probably able to be released at this point.

[coveralls]

Changes Unknown when pulling d01fa5ccea44e80a553342cd38a82ae443f516d6 on neeharv:master into ** on developit:master**.

[neeharv]

Changed the check to use ATTR_KEY. Slightly compacted the iterableToArray function. Code size diff with this PR

branch	minified + gzip	minified
7.1.0	3921	9300
This PR	3999	9514

[coveralls]

Changes Unknown when pulling d0adecfa0ee5adf722ddeb8a1935691f05065309 on neeharv:master into ** on developit:master**.

[developit]

Wow thanks for making those changes @neeharv! I think we're getting close now 👍

[neeharv]

Any update on this? Fwiw this is one of the blockers for us moving over to preact for a major web app! If there are any concerns or changes @developit, happy to discuss and work on that.

[developit]

@neeharv just size is an issue right now. I was holding off on this until the size could be played off against the fairly major reductions in 8.0, though it'd still be amazing if there was some way to further reduce things. Any and all ideas super appreciated, your work here has been really helpful already.

[coveralls]

Coverage decreased (-1.2%) to 96.546% when pulling c3aa1c912f1993fb6a3952cb018e1ca7beca3219 on neeharv:master into 613e128c0ff97cab688f69980d7db568c0d285a2 on developit:master.

[coveralls]

Coverage decreased (-1.2%) to 96.528% when pulling bf75f3f5a2d6ee5d307f3e3a3500b420fb47cb97 on neeharv:master into 861439623d77c1f2cc69371c071c88fca7290d9b on developit:master.