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

Is it really a good idea to support generator function components and iterators/generators as virtual node in general? #127

Open
mcjazzyfunky opened this issue Mar 3, 2020 · 16 comments

Comments

@mcjazzyfunky
Copy link
Collaborator

mcjazzyfunky commented Mar 3, 2020

This is just to open a discussion about the question whether Dyo should really continue to support generator function components and generators (aka. iterators) as children in general.

For this issue, let's define an Iterable as an object that has a special property [Symbol.iterator] whose value is a function that returns something that we call Iterator and that satisfies the corresponding ECMAScript iterator protocol (I think we all know the nature of that protocol).

It's important to know that ECMAScript iterators themselves have a property [Symbol.iterator] whose value is a function that returns an iterator (each time the same (= identical) iterator will be returned!).
That makes a iterator look and behave like a iterable (for example it can be used in a for-of loop).
But there's an important difference between ECMAScript iterators and "normal" ECMAScript iterables:
An iterator can only be traversed once ... while "normal" ECMAScript iterables can be traversed multiple times (virtually infinite times).

Dyo supports iterables as children (which is a really great feature and shall not be changed).
As Dyo currently traverse those iterable children only once it's not a problem that Dyo treats children that are iterators the same way as if they were "normal" iterables.
Therefore without any special logic, Dyo out of the box supports iterators as children as well.

Let's assume somewhere in future Dyo will be splitted into a DEV and a PRODUCTION package to allow additional checks on DEV system like some kind of uniqueness check for keys.
Or maybe a future Dyo will allow its API to be intercepted, similar to what you can do in Preact with this options object and someone wants to add some React-like prop-types validation in userland.
In all such cases, children which are iterators will cause a lot of trouble as soon as the children need to be traversed twice and not just once. Some special userland extensions will not even be possible to be implemented because of that iterator issue.

If Dyo did not allow iterators as children in the first place, nobody would really miss that "feature" (would anyone?) and the win would be that there will be no trouble with iterators any longer and things would be better prepared for future extensions.

Here are a few example of iterators as children which currently are supported by Dyo but should not be supported in Dyo any longer in my opinon:

function* MyComponent() {
  yield 1
  yield 2
  yield 3
}

This behaves exactly the same as the following

function* gimmeOneTwoThree() {
  yield 1
  yield 2
  yield 3
}

function MyComponent2() {
  return gimmeOneTwoThree()
}

Which behaves similar to:

function* gimmeOneTwoThree() {
  yield 1
  yield 2
  yield 3
}

function MyComponent3() {
  const oneTwoThree = gimmeOneTwoThree()

  return <>{oneTwoThree}</>
}

Even if this looks quite unusual, the following example is currently working and should also be working in future, as the component returns an iterable, not an iterator, and therefore will never cause trouble:

function MyComponent4() {
  return {
    *[Symbol.iterator]() {
      yield 1
      yield 2
      yield 3
    }
  }
}
@thephoenixofthevoid
Copy link

According to https://exploringjs.com/es6/ch_generators.html there are:

  1. Generator function
  2. Generator object
    "When you call a generator function genFunc(), you get a generator object". We need to distinguish using generator function as a component (which is nice) and using iterable (in form of generator object or whatever) as an element.

If we stick to the idea, that elements are static and cannot ever change (or at least use it as a mental model), iterable shouldn't be considered as such.

But in all examples I proposed in the issue I have started, I used generator function as a component and form an element before passing it further. When generator function is wrapped as such, as an element It will always represent a sequence of values starting over and over again each time rerendering takes place.

@mcjazzyfunky mcjazzyfunky changed the title Is it really a good idea to support generator function components and iterators/generators as children? Is it really a good idea to support generator function components and iterators/generators as virtual node in general? Mar 4, 2020
@mcjazzyfunky
Copy link
Collaborator Author

mcjazzyfunky commented Mar 4, 2020

@thephoenixofthevoid Again, many thanks for your feedback. Here's my response:

Actually there are use cases that seem to be hard to implement if not using generators.

I've never said that generators should not be used. I've just said that iterators (aka. generator objects) are mutable and may cause trouble if you pass them around, as iterators mutate as soon as you read them.
For each iterator case there is always a possible immutable iterable which has the same capabilities as the corresponding iterator and even more (as the iterable can generate as may corresponding iterators you like, which therefore allows traversing the corresponding data multiple times without mutating the iterable itself).

I consider this library to be one or two levels higher than react

Frankly, I really cannot think of anything that can easily be done in Dyo by using generator function components which is much harder to do in React. Do you have an exemple by any chance?

The rule of thumb is (at least this is what I always propagate regarding the iterator design pattern in general): "If you use iterators locally/internally that's perfectly fine, but in a higher-level API don't pass iterators around, instead pass corresponding iterables around as these are no troublemakers and will not mutate when someoe will read them".

I have to highlight the fact that you are talking about using iterators as an element, but here the key feature is that the generator function (that returns iterator every time it's called) are component.

I've always talked about "iterators as children", would have been better to talk about "iterators as virtual nodes" - therefore I've changed the title of this issue.
Generator function components are use cases in Dyo where the returned virtual nodes are mutating when you read them. That means that virtual nodes are no longer safe to read somewhere in userland without having to fear that reading of the involved virtual nodes will cause unwanted side effects. I really think this is a very unpleasant game changer (and should really be avoided).

And by the way - if you have a look at my examples above (all working properly in Dyo): If the way MyComponent is implemented is allowed, why should MyComponent2 not be allowed? And when MyComponent2 is allowed why should MyComponent3 not be allowed? And at the latest in MyComponent3 you really have iterators as children which can cause trouble in non-trivial use cases - even today.

@thysultan
Copy link
Member

@mcjazzyfunky React supports iterators/iterables as children, how is React's implementation different from your suggestion?

trouble with iterators

Can you post examples that post issues with iterators.

On a secondary note iterators that implement the iterator protocol can a [Symbol.iterator] that would always allow you to re-start the iterator a-fresh, all JS built-in iterators implement this.

@mcjazzyfunky
Copy link
Collaborator Author

@thysultan

Again as definition, to prevent misunderstandings:

With iteratable (aka iterator aggregate) I mean an object that has a property [Symbol.iterator] whose value is a function that returns an iterator. Important: A normal/proper/safe iterable should always return a NEW iterator when iterable[Symbol.iterator]() is called. So you can enumerate its data as often as you like.

An iterator (aka. generator aka. generator object) is by nature a mutable object (will mutate when being traversed) that allows to traverse an enumeration of values. In ECMAScript (in contrast to most other programming systems) an iterator unfortunately also has a [Symbol.iterator] property and therefore looks and behaves like a iterablebut the problem is that calling iterator[Symbol.iterator]() will always return the exact same iterator and NOT each time a new one as you are used by normal/proper/safe iterables.
Therefore its data can only be traversed once.
A generator function function* SomeGeneratorFunction() { ... } will always return an iterator.

React actually does NOT support iterators as children (in newer version). Instead you'll get a warning:

Using Generators as children is unsupported and will likely yield unexpected results because enumerating a generator mutates it. You may convert it to an array with Array.from() or the [...spread] operator before rendering.

Regarding your comment

On a secondary note iterators that implement the iterator protocol can a [Symbol.iterator] that would always allow you to re-start the iterator a-fresh, all JS built-in iterators implement this.

I'm quite sure that's not true (at least not for the usual ECMAScript iterators) .. where does this information come from?

Please have a look at the following demo:
https://codesandbox.io/s/boring-swirles-5z3ci

@mcjazzyfunky
Copy link
Collaborator Author

mcjazzyfunky commented Mar 8, 2020

The main reason I've started this discussion is because this iterator/generator issue had formerly caused some trouble in React (for example: Things were working properly on PROD but not on DEV and other confusing behavior).

See for example this issue:
facebook/react#12995

Like I've said: At least for children, iterators/generators should really not be used as they just cause trouble and will harden or even prevent certain advanced use cases and possible future enhancements (prop type validation in userland or key validation on DEV etc)

If I've understood @thephoenixofthevoid correctly you could argue that even if iterators/generators will not be allowed as children in future any more that this does not necessarily mean that generator function components also have to be forbidden.
That may be true - honestly I do not know one single real-world use case where generator function components really make trouble in Dyo, but nevertheless there really have to be very, very good reasons regarding the big win of generator function components as this opens a lot of questions which currently have not been addressed (for example: Is it okay to call a hook function after a yield statement? - sure, this is currently working fine, but was this really what Dyo's author had in mind?)

And last but not least, please compare virtual nodes and virtual elements in React with Dyo:

In React virtual nodes and elements are completely immutable (at least React will not mutate them), now compare with Dyo's virtual nodes and elements: Not only that they are NOT immutable (as Dyo will modify them and so they only can be used once - I guess this is for performance reasons), with this "generator function components" feature you even have a case (maybe just subtle) where also not even reading is safe any longer ... this is really quite different to React.

@thysultan
Copy link
Member

where does this information come from

A generator object is both iterator and iterable

even prevent certain advanced use cases

Do you have any use cases in mind?

I can get behind removing iterables for element children i.e not supporting h('h1', {}, iterable) but still supporting h(props => iterable).

But we'd need a good good enough reason to create that disconnect, and wether as @thephoenixofthevoid has mentioned there are likewise cases where you would want them to be supported as it is.

@mcjazzyfunky
Copy link
Collaborator Author

mcjazzyfunky commented Mar 8, 2020

where does this information come from

A generator object is both iterator and iterable

That is obviously true but that does not necessarily imply the following:

[...] would always allow you to re-start the iterator a-fresh [...]

Or am I just missing something? ... I still claim that if you only have a usual ECMAScript iterator that the corresponding data can only be traversed once - which is the main problem I am talking about.

The root of all evil is that in ECMAScript an iterator is also an iterable - this is really unusual and a big PITA IMHO, but what do I know....

I can get behind removing iterables for element children i.e not supporting h('h1', {}, iterable) but still supporting h(props => iterable).

To be clear: I am 100000% of the opinion that both h('h1', {}, iterable) and h(props => iterable) is perfectly fine and should be supported to all eternity, but with the very, very important constraint that in these two cases iterable MUST NOT BE AN ITERATOR (what would forbid generator function components as those always return iterators). This is exactly how it is done in React.

https://github.com/facebook/react/blob/238b57f0f712aeddc89604b0dc9c1b13ffb0129a/packages/react-reconciler/src/ReactChildFiber.js#L966

@mcjazzyfunky
Copy link
Collaborator Author

mcjazzyfunky commented Mar 8, 2020

Like said, I do not know a real-world example where generator function components will really cause trouble (maybe in the next 100 years no one will really meet those issues). It's more like a purist view that things should be as simple as possible and if you add complexity there must be really good win.

Anyway, even if not a real-world use case, I think it's not easily understandable why in the following (React) demo that multiplyComponent function would not work in Dyo (=> mutable virtual elements), especially not with generator function components (=> mutable virtual elements + trouble with the fact that iterators can only be traversed once)
https://codesandbox.io/s/determined-lamport-yvvvi

And to repeat my argument against generator function components (see my first comment): If that MyComponent component in my first comment is allowed then I think it's quite natural to conclude that MyComponent2should also be allowed. But then it's really not easy to understand why MyComponent2 is allowed but MyComponent3 should not be allowed?
You do not have such confusion if you just state that iterators shall NEVER be allowed as virtual nodes (as done in React).

@thysultan
Copy link
Member

Anyway, even if not a real-world use case, I think it's not easily understandable why in the following

That has nothing to do with generator functions and is solely related to the virtual node content being virtually the same for each itertation.

@mcjazzyfunky
Copy link
Collaborator Author

mcjazzyfunky commented Mar 8, 2020

[Edit]: Maybe I did not understand correctly and my response below may not really about what you've meant: Still the problem is that generator functions return iterators and exact that iterator is the main problem here, so this has indeed something to do with the generator function (or any other component function that returns an iterator).


No that's not really true: If React would allow generator function components then that multiplyComponent function would (as-is) work with almost all function components but NOT with generator function components because the later will return an iterator and then content would be an iterator which then would be used multiple times which is not working due to the fact that iterators can only be iterated once (not to mention that iterator as children are not allowed in React anyway...).
And this is exactly what I mean: Passing iterators around instead of passing corresponding "normal" iterables around will sooner or later cause trouble which may not be easily understandable ... if you just do NOT allow to pass iterators around then things are so much easier....

The fact that Dyo does not allow virtual elements to be used several times (like React) is not the important point in my example....

@mcjazzyfunky
Copy link
Collaborator Author

mcjazzyfunky commented Mar 8, 2020

Are there really use cases for generator function components that can not accordingly be implemented with something like the following pattern?

function SomeSuperfancyAndForWhateverReasonsKindaLazyComponent(props) {
  // do some fancy stuff here and use as many hook functions as you like

  const generateContent = function* () {
     // yield whatever you like but do not use hook functions here
  }

  return { [Symbol.iterator]: generateContent }

 // // Unlazy aka strict alternative (btw: maybe missing keys could be a minor challange
 // // in the above return pattern):
 // return h(Fragment, ...generateContent())
}

Maybe I just don't see it ....

@thysultan
Copy link
Member

thysultan commented Mar 8, 2020

The issue(on the example posted) is not with generators/iterators but with the way react/dyo deals with shared/immutable virtual nodes, thus a separate issue and not a point of contention for/against iterables/generators.

Per this previous question:

That is obviously true but that does not necessarily imply the following: > Or am I just missing something?

Yes you can still always re-start the generator as demonstrated in the example on the page, but of course you can also create a custom iterable that intentionally does not work like built-in generators but that is another issue.

However generators are always correct because we always re-excute them regardless because they are treated as components.

@mcjazzyfunky
Copy link
Collaborator Author

mcjazzyfunky commented Mar 8, 2020

@thysultan Sorry, but I really have to be blind ... where exactly on that page is mentioned that you can "always restart the generator" if the generator (aka "generator object" , which is an "iterator" aka "iterator object") itself is the only thing you have?
Of course if you have the generator function itself you can always call that function to get a new iterator object, but that is not what I am talking about.

let aGeneratorObject = function* () {
    yield 1 
    yield 2 
    yield 3 
}()

console.log([...aGeneratorObject]) // working => [1, 2, 3]
// would be good to kinda "restart" the generator object here
console.log([...aGeneratorObject]) // not working => []

https://jsbin.com/notaneqipu/edit?js,output

@thysultan
Copy link
Member

Yes your right, i was mistaken on that point. i.e [...aGeneratorObject[Symbol.iterator]()] doesn't re-start it.

@mcjazzyfunky
Copy link
Collaborator Author

mcjazzyfunky commented Mar 8, 2020

Okay, then to come to an end:

  1. I would then suggest that iterators as children are not allowed any longer ... but iterators are still allowed as direct (!) return value of (generator) component functions.

  2. Maybe a bit more information would be nice here
    https://dyo.js.org/component.html#generator

[Edit: No need to discuss the following ... just saying that maybe it would be nice if the documentation said what's allowed and what's not regarding generators]

function DoNotKnowWhetherAllowedOrNot1() {
  // Do I have to use keys here (like for other iterables)
  // or are keys completely ignored in this special case?
  yield (<X>1</X>)
  yield (<X>2</X>)
  yield (<X>3</X>)
}

function* DoNotKnowWhetherAllowedOrNot2() {
  yield 1
  // Are hooks allowed in future after first yield?
  const [two] = useState(2)
  yield two
  yield 3
}

function DoNotKnowWhetherAllowedOrNot3() {
  // Should this also be allowed in future? (IMHO it should not, but anyway....)
  
  // do something fancy here  

  function* generateOutput() {
    const [whatever] = useState('whatever') // IMHO hooks should not be allowed here any longer
    yield whatever
    // [...]  
  }

  return { [Symbol.iterator]: generateOutput }
}

@thysultan
Copy link
Member

thysultan commented Mar 9, 2020

Do I have to use keys here (like for other iterables

You only need to use keys for computed children i.e data.map(v => <x>{v}<x>) for example [<x>1</x>, <x>2</x>] does not need keys but condition ? [<x>1</x>, <x>2</x>] : [<x>3</x>, <x>4</x>] might need keys as the it is conditionally computed.

Are hooks allowed in future after first yield

Yes.

Should this also be allowed in future

It probably already is, given that resembles what generators do when you do yield* otherGenerator, so that example would be identical to the previous one.

I would then suggest that iterators as children are not allowed any longer

I ok with this, though in it's current iteration the implementation that handles both is more streamlined or simpler that the proposed.

@thysultan thysultan assigned thysultan and unassigned thysultan Sep 10, 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

No branches or pull requests

3 participants