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

this.refresh() failed to update DOM #27

Closed
TechQuery opened this issue Apr 18, 2020 · 22 comments
Closed

this.refresh() failed to update DOM #27

TechQuery opened this issue Apr 18, 2020 · 22 comments

Comments

@TechQuery
Copy link

TechQuery commented Apr 18, 2020

I wanted

I tried to use Crank with MobX, and then I wrote observer() adopter for Crank.

I wrote

https://codesandbox.io/s/crank-router-7f5o3?file=/src/observer.ts

I got

Every invoking of this.refresh() created correct VDOM node, but the real DOM updated nothing...

image

@brainkim
Copy link
Member

brainkim commented Apr 18, 2020

Thanks for messing around with Crank!
Unless you’re working with global state, or even in that situation, stateful components should be defined with generator functions, not regular functions. This is because stateless components throw everything away between renders. So when you updated the node variable in your component, it worked the first time, but when the hash was changed, the autorun callback was called in the context of that old execution, and the component started over with a new autorun callback where node was again undefined. If this doesn’t make sense, try putting a console.log statement right before the return of the function returned by observer.

I got your example working here: https://codesandbox.io/s/crank-router-n25wq

One thing I would caution against is calling components directly, as you’ve done in your example. Components can be sync functions, async functions, sync generators or async generators, so it’s not a matter of simply calling the function and getting elements. Hover over the node variable and see its inferred type if you want to see what I’m talking about. You have to handle all these cases if you want to call components directly, which is why I think it’s preferable to let Crank call your components for you. You can do this by not calling the component but creating an element with it.

Let me know if any of this is unclear,
Happy experimenting!

@TechQuery
Copy link
Author

@brainkim Your sandbox crashed in my Chrome 81:
image

@brainkim
Copy link
Member

That looks like an error that was thrown by the mobx API? I’m not too familiar with how mobx works. Surprisingly, changing the hash still works somehow :)

@lishine
Copy link

lishine commented Apr 18, 2020

The observer() is intended to wrap a react component

@TechQuery
Copy link
Author

@brainkim I don't think so. Every error throws in MobX call stack will be caught by MobX, and then show a debug log.

Could you give me an example code of "change hash"?

@TechQuery
Copy link
Author

@lishine It's just an adopter, not specific for React, for example:
https://github.com/EasyWebApp/WebCell/blob/v2/MobX/source/index.ts

@lishine
Copy link

lishine commented Apr 19, 2020

@lishine It's just an adopter, not specific for React, for example:
https://github.com/EasyWebApp/WebCell/blob/v2/MobX/source/index.ts

My bad

@benjamingr
Copy link

Hey, let me know if you need any MobX specific help here - writing an adapter for MobX should be relatively simple although I think reactivity should be first class.

@TechQuery
Copy link
Author

@brainkim @benjamingr
I realize that an Observable Object implemented Symbol.asyncIterator interface may be the best State Manager for Crank, so I show you a Router example:
https://codesandbox.io/s/crank-router-reactive-3up7i?file=/src/Router.tsx

@benjamingr
Copy link

That package sort of misses the point of push vs. pull :D

That said MobX observables themselves should be async iterable and if they're not - we can add that there.

@TechQuery
Copy link
Author

That package sort of misses the point of push vs. pull

Could you make this clearer by some example codes?

@benjamingr
Copy link

@TechQuery yes and sorry that comment by me was kind of rude. "Misses the point" isn't a very charitable way to speak,

Rx is a push based abstraction (like MobX and Vue observables and they are actually the same thing).

Async iterators are iterable they are a pull based abstraction. I go a bit into the differences here (I also have the slides) - but basically:

  • Observables like Rx and MobX are push based. You subscribe to them once (by calling .subscribe) and then the producer has all the control. They push data to you whenever they want. They are (as Erik who invented them told me) "hyper strict" - and the opposite of lazy.
  • Iterables are lazy, very lazy - the consumer has all the control. With an async iterator the consumer has all the control. They decide when to ask for the next item - it is not "pushed" to them.

The advantage of observables and "push based" is that the fact it's simpler and the producer has more control. Pull based means things support back-pressure and building pipelines is possible.

Namely, any implementation of observables with async iterators would just be a "pump" and would be very confusing in terms of back-pressure and it would also be a lot slower than a function call. After all the simplest push abstraction is a function call - and observables are in principle just:

class Observable { constructor(subscribe) { this.subscribe = subscribe; } }

It's a way to defer a function call to later (when you call subscribe) and then that function calls you eagerly.

@TechQuery
Copy link
Author

@benjamingr
Class Observable of iterable-observer package has a simple constructor(), too:
https://github.com/EasyWebApp/iterable-observer/blob/master/source/Observable.ts#L29-L34

@benlesh also built a package to use RxJS withfor await (): https://github.com/benlesh/rxjs-for-await

In old proposal edition, the return value of Async Generator was an Observable, so I think it makes sense to build an Iterable Observable object.

@benjamingr
Copy link

RxJS observables are also async iterable as far as I know.

If you're reading oldschool proposals - compositional functions is funner than for.. on :D

@TechQuery
Copy link
Author

@benjamingr
Although Iterables are Pull mode, combining it with Crank, makes source code linear, and then Crank will update components automatically:

async function* Component() {
    const stream = makeStream()

    for await (const state of stream)
        yield <div>{state}</div>;
}

If we use typical Observables, manual updating must be done:

async function* Component() {
    var state;

    observable.subscribe(data => {
        state = data;
        this.refresh();
    });

    while (true)
        yield <div>{state}</div>;
}

In my opinion, the target of Generator & Async syntax is reducing count of Callbacks.

@benjamingr
Copy link

What I was saying is that the above example (of for... await) would work with an Rx observable as well :]

@TechQuery
Copy link
Author

TechQuery commented Apr 23, 2020

@benjamingr I didn't know RxJS can do this, so I wrote my own with TypeScript, I may try it later, thanks.

@TechQuery
Copy link
Author

@brainkim @benjamingr I finished the first Beta version of crank-router

@ryhinchey
Copy link
Contributor

ryhinchey commented Apr 24, 2020

@TechQuery this is cool. I’ve been working on a router implementation myself using the history module that powers react router and works in the browser and node https://github.com/ReactTraining/history. I’ll take a look through your module and will raise some issues/PRs if you’d like!

This is my initial router code https://codesandbox.io/s/charming-goodall-p2mo7

@TechQuery
Copy link
Author

@brainkim
I wonder, why is it no need to invoke this.refresh() while using Crank with an Observable implemented Async Iteration interface?

@brainkim
Copy link
Member

brainkim commented Apr 30, 2020

@TechQuery There actually isn’t a need to call this.refresh with async generator components. When a crank component returns an async iterator object, it will continuously pull values from it until the component is unmounted. This means you could theoretically write code like:

function Component() {
  return new History().listen(this).map((routeData) => (
    <div>{routeData}</div>
  ))[Symbol.asyncIterator]();
}

I dunno if this code would work in your example, this is just to give you an idea of what you could do with async iterators. The two caveats I have are as follows:

  1. you must always yield a value at least once every time you pull props from this, or Crank will throw an error. This means you couldn’t use observable combinators like bufferWhen.
  2. you must return an async iterator not an async iterable. Crank uses duck-typing to look for components which return an object with a next method to determine if that component is a generator/async generator component, because otherwise you wouldn’t be able to return arrays or strings from components without them being exploded so that they yield each element or character per update.

Happy experimenting! If all questions you have are resolved, then you may want to close the issue. If on the other hand, any of this didn’t make sense, ask again and I will try to be more concrete (I’m trying to answer as many issues as I can right now so I’m moving quickly).

@brainkim
Copy link
Member

brainkim commented Jul 2, 2020

Closing for housekeeping purposes. Feel free to continue the discussion.

@brainkim brainkim closed this as completed Jul 2, 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

5 participants