Add support for all iterable children, not just Arrays

[neeharv]

This will fix preactjs/preact-compat#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 preactjs/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" ***@***.***> 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 <#476 (comment)>, 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]

This is great and I'm good with the changes. I will definitely play with things to try and get file size down though in order to make sure this isn't a hit for people not using Iterables. Thinking a forward loop with splice() over an inclined version of your to array function.

I genuinely don't mind riffing on things for size given that there are unit tests I can monitor. Let me know if you have ideas for that though.

[robertknight]

The test for Symbol support will throw an error in browsers where it isn't defined. Otherwise LGTM.

[robertknight]

isFunction(Symbol) triggers a ReferenceError if Symbol is undefined. Using typeof Symbol === 'function' is OK however.

[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]

Coverage decreased (-1.2%) to 96.528% when pulling 4ca9ff9 on neeharv:master into 498c432 on developit:master.

[coveralls]

Coverage decreased (-1.2%) to 96.528% when pulling cd468eb on neeharv:master into 523c397 on developit:master.

[JustinMGaiam]

@developit thanks! I look forward to version 8 so we can start looking at Preact as replacement for React.

[coveralls]

Coverage decreased (-1.2%) to 96.528% when pulling 353af03 on neeharv:master into bdcb6b5 on developit:master.

[robertknight]

The tests are failing after merging in changes from master. I don't suppose you'd be able to take a look @neeharv .

@developit what is your plan for this PR and Preact 8? Did you manage to make sufficient size savings elsewhere?

[developit]

I've made some savings yes, but since we are aiming for 2kb it's still tight. I might have a way to cut down further though. If you're available on slack, ping me

[neeharv]

Will take a look over this weekend and get the tests passing. Would love to have this in v8!

…

On Sat 25 Mar, 2017, 2:22 PM Sampo Kivistö, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/h.js <#476 (comment)>: > +const hasSymbolSupport = isFunction(Symbol) && Symbol.iterator; uh huh, I dont know how I have missed this comment. Its now failing in IE, lets fix it for next release. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#476 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABsS4W3nobZf-s5xx69WRghPffzWmCWnks5rpNXXgaJpZM4LW0Gm> .

[developit]

@neeharv Agreed!

[neeharv]

Did something in the latest master change wrt karma/phantomJS? I think I fixed the failing issue, but didn't touch any test code. Now phantomJS hangs completely when running the test suite, on local as well as on Travis. My knowledge of PhantomJS and Karma is limited, so any help would be appreciated.

[developit]

I've been seeing this in other repos, I think it's a patch release of a dependency that just broke everything (annoying). FWIW I usually use the Chrome runner:

npm i -D karma-chrome-launcher

npm run test:karma:watch -- --browsers Chrome

[neeharv]

I've been trying to merge the latest master into this and finally get this done, but I've run into an issue.

In h.js, we check if the child is an array, using

if ((child = stack.pop()) && child.pop!==undefined) 

whereas earlier there was an instanceof Array check. The problem with this is that third party collection libs may implement a .pop() method, (for eg- Immutable.List) and then this breaks.

Suggestions on the best way to fix this? I have the code working fine with native collection objects like Set / Map using the ES6 iterator, but hard to find a fix for libs like Immutable without going back to the instanceof check.

[developit]

Hmm. Might have to switch to Array.isArray() even though the performance is much worse :(

[neeharv]

I can't seem to figure the browser tests out - everything else works fine in terms of testing, using phantom or chrome with karma just hangs the test and errors, locally or in CI. I can't see the exact errors, so don't know how to fix it. @developit do you think you could take a look? Everything else looks good.

I'd really love to get this in, will help us switch insider.in over to preact!

[Emilios1995]

I also need this feature in order to switch to preact. Any new progress?

[neeharv]

Ping. Any help @developit on this? Unable to figure how to get the tests to pass with Karma launcher, it just hangs for me.

[developit]

The karma issue should be resolved thanks to downstream updates.

I know this is an unusual request, but if anyone is interested in Iterables as children, I'd ask you to please join the Preact slack group. We have some stuff to talk about!

[mq2thez]

Note that this unfortunately a blocker for Etsy migrating to Preact. While we would very much love to drop Immutable as a dependency, doing so is a long-term project due to the size of our codebase.

[marvinhagemeister]

@mq2thez we're looking into it 👍