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

RFC: Hooks #87

Closed
thysultan opened this issue Jan 7, 2019 · 59 comments
Closed

RFC: Hooks #87

thysultan opened this issue Jan 7, 2019 · 59 comments

Comments

@thysultan
Copy link
Member

thysultan commented Jan 7, 2019

Similar to React Hooks. RFC discussion.

The differences being

  1. The use of a single setter/getter stream instead of a tuple. So value = use(value) instead of [value, setValue] = use(value). This allows us to use live values to avoid the age old problem of stale data. That is value() would always return the current value.
  2. The unification of hooks and effects into a single use API. i.e use(value) and use(value, callback) instead of useState and useEffect.
  3. Support for unmount animations as can be archived with componentWillUnmount. Without which hooks wouldn't have full feature parity with lifecycle methods.
  4. ...and passing [dependencies], props, state, context to the effect callback to effectively avoid being tied to closures.

An example of the aforementioned might look like the following:

import {h, use, render} from 'dyo'

function Counter (props, {refs, value = use(0)}, context) {
	try {
		return h('div', {ref: refs},
			h('button', {onClick: e => value(value + 1)}, '+'),
			h('button', {onClick: e => value(value - 1)}, '-'),
			h('h1', {}, value)
		)
	} finally {
		useAnimation(value)
	}
}

function useAnimation (value) {
	return use([value], ([value], props, {refs}, context) => {
		return () => {
			return refs.current.animate([], {}).finished
		}
	})
}

Point 3 arises the question of how do we handle the case when multiple effects try to register unmount animations: Do we 1. Register all the animations? or 2. Register the last animation?

@thysultan thysultan changed the title Hooks RFC: Hooks Jan 7, 2019
@mcjazzyfunky
Copy link
Collaborator

mcjazzyfunky commented Jan 7, 2019

@thysultan Really great that you are planning to add a hook API to Dyo. Thanks.

Just a comment to point 1:
You know, in the past, I've always been a fan of that setter/getter streams.
But frankly, now, I'm not really sure any longer, whether this really solves more problems that it causes trouble/confusion.

Please see the following example:

// React - everythings completely understandable here
function Counter(props) {
  const
    [count, setCount] = useState(0),
    { label, onChange } = props,
    prevLabel = usePrevious(label),
    prevOnChange = usePrevious(onChange),
    prevProps = usePrevious(props),
    prevCount = usePrevious(count)
    ...
}

function usePrevious(value) {
  const ref = useRef()

  useEffect(() => {
    ref.current = value
  })

  return ref.current
}
// Dyo - things are a bit confusing here, I think...
function Counter(props) {
  const
    count = use(0)
    { label, onChange } = props,
    prevLabel = usePrevious(label),
    prevOnChange = usePrevious(onChange), // onChange is a function
    prevProps = usePrevious(props),       
    prevCount = usePrevious(count)        // count is a function
    ...

  // Some confusing things
  // * "count" is a function - is "prevCount" is a number or a getter function?
  //   You cannot guess the type of a value by its name any longer.
  // * How can "usePrevious" know that "onChange" is a function
  //   which shall be treated as a value while "count" is a function
  //   which shall be treated as a getter function? Means, "usePrevious" is
  //   not as easy to write and not as easy understandable as in React...
  //   Of course you can just use: prevCount = usePrevious(count()) ... but still confusing...
}
 

@thysultan
Copy link
Member Author

thysultan commented Jan 7, 2019

@mcjazzyfunky From the example i can't pin point in what context this would be a problem. Can you elaborate? i.e how are you using the values, and how is this different from React.

As an aside if for example use(use(0)) returned the passed value. Could rewriting usePrevious return to return use(ref.current)() establish the guarantee that it always returns a value regardless of the type?

In addition, assuming the tuple approach [value, setter]. How would you avoid stale data given the move towards a more async world with suspense et-al, and how would you pass this onto effects(regarding point 4).

As it stands dependencies are single live values so ([a, b, c], props, state, context) doesn't feel too off compared to ([[a, setA], [b, setB], [c, setC]], props, state, context), or is the premise of passing the dependencies as arguments to the callback not the right direction?

My reasoning was that this would widen the breath of compostability afforded to effects in addition to the aforementioned note about closures in point 4.

@mcjazzyfunky
Copy link
Collaborator

mcjazzyfunky commented Jan 7, 2019

@thysultan Like I've said I'm just "not really sure" ... maybe my concerns are a bit overstated.
Just saying if you have a variable "count" at a certain point within the render function then with React you normally know the type of that variable without having to look at the previous code or using some IDE tooltips ... in Dyo you would not know whether it is a number, a getter or a getter-setter-hybrid.
But this is not a showstopper of course (libs like "knockout" or "flyd" use a similar syntax and nobody has problems with it there).

But what's really a bit odd in my opinion is the following: Let's say again at a certain point in the render function you have that variable"count". If it comes from "use" it's a stream if it comes from "props" it's a number. Should the source of data really be THAT important (different types depending whether it comes from state, from props or from context)? If the data source is state than you do NOT have that "stale data" problem and you can pass the data stream easily to async functions. But if the data source is "props" or context then your STILL have that "state data" problem and cannot pass the data that easily to async functions, am I wrong? (maybe some "useStream(...)" hook could be a solution).

But really no need for a larger discussion just because of that "value vs getter/setter-stream" thing ... if you use "getter/setter-streams", I'm fine with that of course 😃

Regarding that depenency callback argument at point 4: Type inference support will not be much fun in TypeScript/Flow with that "use" function, wil it?

@Zolmeister
Copy link

Zolmeister commented Jan 7, 2019

My experience with getter-setter functions (working with Mithril.js years ago) was poor.
I ran into 'variable naming confusion', where (like @mcjazzyfunky) mixing of getter-setters with regular variables leads to hard-to-follow code.
Special-naming (e.g. var -> varProp) solves it, but looks ugly

  1. based on the above, I disagree with this move
  2. My (toy) examples so far do not use parameters for useEffect, therefore the extra first parameter (from merging with useState) is useless (I'd predict mostly null)
  3. no comment
  4. seems harmless (doubt I'll use them)

I'd like to note that useEffect should not run server-side (#86)

@thysultan
Copy link
Member Author

Regarding that dependency callback argument at point 4: Type inference support will not be much fun in TypeScript/Flow with that "use" function, will it?

You could type define it as:

interface Use<T, Props = {}, State = {}, Context = {}> {
    (value: T, callback?: (Value<T>[], Props, State, Context): (void | (): Promise<any>))
}

My (toy) examples so far do not use parameters for useEffect, therefore the extra first parameter (from merging with useState) is useless (I'd predict mostly null)

@Zolmeister Yes, React uses the second argument to signal dependencies, where implicitly it is an empty array(no dependencies). In contrast this forces you to explicitly mention dependencies if there are any, which is either a pro(is explicit) or con(is explicit?) depending on what you want to do.

I'd like to note that useEffect should not run server-side.

I agree.

@Zolmeister
Copy link

Zolmeister commented Jan 7, 2019

I can also see benefits to re-ordering the arguments for useEffect (e.g. lodash/fp)

Instead of merging useEffect/useState, how about useState/useMemo?

Edit: ah, I take it back useMemo returns a value, not a tuple. I'm starting to see the value of having separate methods here (akin to map, filter, reduce, but for hooks)

@thysultan
Copy link
Member Author

I agree overloading use might not be a good idea considering there maybe more use* hooks in the future like useBoundary in addition/akin to useContext specifically for errors handling etc.

Basic Hooks

  • useState
  • useEffect
  • useContext

Additional Hooks

  • useReducer (Do we need this?)
  • useCallback (Do we need this initially?)
  • useMemo (Do we need this initially?)
  • useRef (Do we need this considering we can just use any object i.e ref = {})
  • useImperativeMethods (Borderline useEffects, can we merge into useEffect)
  • useLayoutEffect (Borderline useEffects, can we merge into useEffect)

@Zolmeister
Copy link

  • useReducer probably not
  • useCallback probably not
  • useMemo, definitely need this
    • Everything that you would previously have put into a constructor, you should put here (to avoid expensive calculations)
  • useRef probably not, useState({current: null}) seems equivalent
  • no preference
  • no preference

@thysultan
Copy link
Member Author

thysultan commented Feb 11, 2019

I think we can ship the following primary and secondary hooks:

Primary

  • useState
  • useMemo
  • useEffect
  • useLayout
  • useContext
  • useBoundary

Secondary

  • useRef
  • useReducer
  • useCallback

It's debatable whether we should include useCallback, it does though have uses that cannot be mimicked with useMemo.

All of the above are implemented in the hooks branch.

@mcjazzyfunky
Copy link
Collaborator

@thysultan What about useImperativeHandle (fka useImperativeMethods)? Somewhere above you've mentioned: "[...] can we merge into useEffect [...]".
In React you need forwardRef (which is not available in Dyo) if you want to use useImperativeHandle.

@thysultan
Copy link
Member Author

thysultan commented Feb 13, 2019

I was mistaken as to it's purpose, that said useImperativeHandle looks niche. Is there any general use case for it beyond decorating refs ref.current handle?

...The function served by forwardRef is the default behaviour as of the hooks branch, exposing a forwardRef API would be for the most part sugar we can avoid. The same is true for React.lazy in contrast to Dyo's first-class support for thenables and consequently dynamic imports.

@mcjazzyfunky
Copy link
Collaborator

mcjazzyfunky commented Feb 13, 2019

I think focus() is a common example for useImperativeHandle (see https://reactjs.org/docs/hooks-reference.html#useimperativehandle).
Another example may be a usual DateField component which opens a calendar sheet to select the date by mouse click. Whether the calendar sheet is shown or hidden is normally handled completely by the DateField component itself. Nevertheless there are use cases where you want to close the calendar sheet from outside. For this use cases it may be useful if the DateField component has a imperative method close() which can be called from outside.
Another example may be a table component that allows to select rows. Which rows are selected is handled completely internally. Nevertheless in some cases you want to unselect all rows from outside. In this case a imperative method unselectAllRows() may be useful.

@thysultan
Copy link
Member Author

thysultan commented Feb 13, 2019

Interesting.

I think useRef is versatile enough as a primitive(at least in Dyo) that you can do this with the function initialiser pattern.

For example:

// Dyo
function FancyInput(props) {
  const ref = useRef(props => {
  	props.ref.current = {focus: () => ref.current.focus()}
  })

  return <input ref={ref} ... />
}

In React this would be the equivalent of.

// React
function FancyInput(props, ref) {
  const inputRef = useRef();
  useImperativeHandle(ref, () => ({
    focus: () => {
      inputRef.current.focus();
    }
  }));
  return <input ref={inputRef} ... />;
}
FancyInput = forwardRef(FancyInput);

Is there anything i'm missing?

@mcjazzyfunky
Copy link
Collaborator

@thysultan Ooooops, sorry for accidentally closing the issue...
Anyway, the Dyo solution that you've provided above regarding those imperative focus method is really interesting.
But that means that #77 is not considered a bug in Dyo (I personally always thought a virtual element should look something like { type, props, key, ref } without key and ref being part of props - but maybe I am just wrong).
Also it means that in future Dyo´s API will be much less similar to React's API than DIOs, correct? ... would not be a problem for me, btw...
Just asking, as DIO was out-of-the-box much more React-like than all other VDOM-based React competitors so it always seemed to me that API-compatibility was a main goal of DIO.

@thysultan
Copy link
Member Author

thysultan commented Feb 13, 2019

The main reason i can see React still removing "ref" from props is from class components where passing a ref to a class component in react would invoke the ref with the instance of the class first, this meant that you would either have to live with the ref getting invoked twice once for the component instance, then the DOM node, or remove the ref from props to avoid this, i might be wrong but this is a similar situation in which DIO was in previously.

Going all-in with function components makes this a none-issue since instances effectively don't exist. Dyo will take liberties where it makes sense but is still at heart very close to a "React-like" API, i.e similar examples include altering the naming of useLayout instead of useLayoutEffect. useContext returning a tuple [value, dispatch] instead of a single value for better ergonomics around updating context. useBoundary for serving the purpose of a primitive ErrorBoundary hook, and passing the arguments in useEffect, useLayout to the function i.e useLayout(([a, b, c]) => {}, [a, b, c]) allowing a case for non-closure effect functions.

@mcjazzyfunky
Copy link
Collaborator

Even if those cases may be very rare, to have key and ref as properties in props seems a bit of a troublemaker to me if you use <SomeComponent ... {...props} .../> somewhere. An alternative may be to remove key and ref from props and pass ref as second parameter to the component function, anyway....
Just for information: Both React's and Inferno's createElement functions remove key and ref from props, while Dyo and Preact do NOT remove them => seems to be a draw 😃

Oh, and thanks for the details about the API differences to React.

Means, my above asked questions are all answered now. Thank you very much.

@thysultan
Copy link
Member Author

The only prop i can see being potentially destructive is "key", though considering that if the parent that's passing the key, changes the key, then the parent would be destroyed in either case.

The only other issue might be in the order of de-structuring when you are suppling a key i.e <SomeComponent key="1" {...props}/> vs <SomeComponent {...props} key="1" /> being the only other issue you might come across depending on whether the JSX transformer already alleviates this concern.

Where there any other scenarios you had in mind?

@mcjazzyfunky
Copy link
Collaborator

mcjazzyfunky commented Feb 13, 2019

I think we will also have trouble in TypeScript, as soon as the dyo.d.ts will be available somewhere in future:

Just assume that someone has written useImperativeHandle in userland corresponding to your proposal above. Also assume that she also writes a typical simple counter component, which shall have an imperative method reset(n: number):

type CounterProps = {
  initialValue?: number,
  label?: string
}

// We ignore this type information here (as it is not really that important here)
// type CounterMethods = {
//   reset(n?: number)
// }

function Counter(props: CounterProps) {
  useImperativeHandle(
    props.ref, // <------------ TYPE ERROR !!!!!!!!!!!!!!!!!!!!!!!!!
    () => {....} // add imperative function 'reset(n)' here
  ) 
  ....
}

----- Edit --------------------------

While a properly typed solution could look like:

type CounterProps = {
  initialValue?: number,
  label?: string
}

type CounterMethods = {
   reset(n?: number)
}

function Counter(props: CounterProps, ref?: Ref<CounterMethods>) {
  useImperativeHandle(
    ref,
    () => {....} // add imperative function 'reset(n)' here
  ) 
  ....
}

@thysultan
Copy link
Member Author

Is there any reason that the following amendment wouldn't alleviate that TypeScript error?

type CounterProps = {
  initialValue?: number,
  label?: string,
+ ref: Ref<CounterMethods>
}

@mcjazzyfunky
Copy link
Collaborator

mcjazzyfunky commented Feb 14, 2019

Maybe I'm completely biased regarding that key/ref thing ... maybe at the end of the day it may just be a matter of taste.
It seems to me that key and ref are only pseudo-props which are only passed as props when calling createElement because it's syntactically the most consice way and with JSX there is no other option than to pass them as props. But in React after the virtual element is created refand key are no props any longer and you do not have to care about them any longer when dealing with the props object.
Adding ref?: Ref<CounterMethods> to type CounterProps in the example above would work of course, but after that ref is a 100% "normal" prop, which has to be completely handles like any other prop.
Now ref is a "normal" prop which has to be declared in the component's Prop type whilekey is still some kind of "alien" prop which must not be declared (but yet it will be part of props).

Nothing of that is a big deal and all of the very rare above mentioned issues can be handled easily in some way, but nevertheless it does not feel like an especially good solution to me.

Hope the following example is not too confusing, but at least it's real world example:
When I develop a general purpose component library (containing mostly leaf/dump/presentational components) then I want to write them in TypeScript with very strict compile time checks for props, but I additionally also want runtime prop validation for those who use these components in JavaScript directly (and not TS). Moreover I do not want to use the prop-types library for the prop validation, instead I want to use a general purpose validation library (which has far more validator functions available and can also be used for completely other non-UI validation stuff in the apps). Last but not least I do not want to use forwardRef(...) explicitly.
All of that leads to a helper function called defineComponent which is used here (everything is still experimental, btw):

https://stackblitz.com/edit/react-ts-wdp64s

Be aware that ref is just passed as second argument to the render function and can easily be passed to useImperativeHandle.
Also be aware that there is a 100% symmetry between the keys of type CounterProps and the configuration at properties: which will be enforced by the type sytem.
If I add ref?: ... to type CounterProps I also have to add an additional key/value pair at properties: which is not a big deal of course but nevertheless a bit annoying.
Of course I could modify the type declarations that it would not be necessary any longer to add ref at properties: but then the keys of type CounterProps and properties: are not symmetric any longer.

To make a long story short: If key and ref are part of props then you get some additional subtle conceptional noise, which I think is not really necessary.

@thysultan
Copy link
Member Author

I can see this going both ways, i.e That there's an inconsistency divide in declaring <Foo value={} key={} ref={}> props but only being able to access value from the props objects.

I can understand why this was done in the presence of class instance refs but conceptually a pass-through heuristic appears less magical than the runtime explicitly deleting these props and introducing API's to deal with the issues this might cause for some types of components, a thing we've previously seen mitigated by runtime heuristics for example innerRef in Styled Components.

@mcjazzyfunky
Copy link
Collaborator

Okay, then handling of ref and key stays as-is ... like said, maybe at the end of the day just a matter of taste ... and except for the case of #77 (which can be handled easily) there have never been real problems in DIO regarding this 😃

@thysultan
Copy link
Member Author

I'm hoping that goes into the same isle of "consider props to be immutable" – "always put destructing first". Which consequently is identical to how it would work with plain JavaScript objects {val: 1, ...obj}.

@thysultan
Copy link
Member Author

Batching out of band updates

Consider the following:

const Example = props => { console.log('render')
	const [state, dispatch] = useState(props)

	// example 0 (batched)
	useEffect(props => {
		dispatch({children: 'Hello'})
		dispatch({children: 'Hello World'})
	}, [])

	// example 1
	useEffect(async props => {
		await timeout(1000)
		dispatch({children: 'Hello'})
		dispatch({children: 'Hello World'})
	}, [])

	// example 2
	useEffect(props => {
		setTimeout(() => {
			dispatch({children: 'Hello'})
			dispatch({children: 'Hello World'})
		}, 1000)
	}, [])

	return state.children
}

render(Example, target, (current) => {
	console.log('complete')
})

As it stands boy React and Dyo would only batch example 0. Both example 1 and example 2 happen out of band(outside of the effect) and will each initiate two updates(render).

In addition the log complete will log before either of these out-of-band dispatches are complete.

What should we do?

  1. Follow React, leave it as is – independent un-batched updates.
  2. At least support example 1. since we have a handle to register to when it is resolved. This would effectively both make example 2 batched and resolve the complete log at the right time once its updates are resolved.
  3. Try to batch example 2 in addition to example 1. This however would have a caveat of being impossible to get the complete log to happen at the right time but we can at the least get batching to happen.

@Zolmeister
Copy link

2 seems valuable, +1

@thysultan
Copy link
Member Author

Event iterables.

Considering we can attach multiple event listeners to a single event with arrays:

return h('button', {onClick: [a => 'Hello', b => dispatch(e)]}, 'Button')

What model should we opt for.

  1. a should be the event object, b should be "Hello" aka the next in the chain receives the return value of the last.
  2. a and b should both be the event object regardless of the return value.

Number 1 is what is currently implemented in Dyo(DIO used heuristic 2) but i'm questioning what is the cost-benefit between the two.

@Zolmeister
Copy link

I didn't even know it was supported.
I use _.flow(a, b) and _.over(a, b) (lodash) to accomplish 1 and 2 without multiple listeners.
That said, ergonomically 2. makes more sense, and is also my most common use case

@thysultan
Copy link
Member Author

thysultan commented Feb 20, 2019

@Zolmeister In it's current form the implementation details of useCallback don't have the problem of "invalidates too often" because it does not need to use the dependencies array for invalidation, aka useCallback never needs to be invalidated it is always the same function returned like useState and useReducer regardless, we only change the pointer to the callback passed.

That is

// render 1
const fn = useCallback(event => event, [true])
// render 2
const fn = useCallback(event => event, [false])

Event though the dependencies values have changed, would always return the same function, but still update the pointer to the callback that is invoked when called. So a sort of best of both worlds.

I think your proposal hurts inline functions too much (repeating arguments twice)

Yes the is one side-effect of this, we could push them to the end to solve this?

function handleClick (event, props, [state, dispatch]) {
	dispatch(state.a + props.b + event.type)
}
// ...
useCallback(handleClick, [state, dispatch])
useCallback(event => event)

Your use case could be solved by using currying e.g.

Any user-land solution would rely on always creating closures on every render. When you're optimisation the final straws of your app there may come a time when this is where you might want to exert your final efforts, being able to archive this when required is an equally strong motivation behind this.

@Zolmeister
Copy link

Any user-land solution would rely on always creating closures on every render

I don't understand how your example works. How does it know what props, state, dispatch are? Isn't closure creation always required?

@thysultan
Copy link
Member Author

I updated the example(there was a error with missing dependencies tuple).

That said the props argument is similar to the event argument, the event dispatcher passes it to the function. The [state, dispatch] is the tuple passed to useCallback(handleClick, [state, dispatch]).

The callback returned from useCallback is only ever created on the first render(mount), subsequent renders(update) always re-use this. This callback calls whatever callback is currently stored with (a, b, T) where a and b are the arguments it receives and T is the tuple argument passed to useCallback(handleClick, [tuple]).

I'm sure you can probably re-create this with a user-land solution(using useState, useRef, useMemo) that avoids the "creating closures on every render" clause. But for something as primitive as this it should probably be part of the library.

@Zolmeister
Copy link

I'd be in favor of removing the dependencies argument all together and having useCallback bind arguments.
e.g.

function fn(a, b, c, e, f) { }
cb = useCallback(fn, a, b, c)
cb(e, f)

@thysultan
Copy link
Member Author

In practice this is what it does.

cb = useCallback((a, b) => {  console.assert(a + b == 3) }, 1)
cb(2)

However it doesn't allow arbitrary argument arity. Which is why a tuple was used to mimic arbitrary arguments.

cb = useCallback(([a, b], c) => { console.assert(a + b + c == 6) }, [1, 2])
cb(3)

@Zolmeister
Copy link

Why not support arbitrary argument arity?

@thysultan
Copy link
Member Author

When considering the common case of attaching the callback as an event, handlers are passed (event, props) as arguments they would receive, that translate to (event, props, [a, b]) or ([a, b], event, props) depending on whether we pin the dependencies to the head/tail of parameters.

In the case of pinning them to the head(([a, b], event, props)), we can support support arbitrary arguments because args naturally flow from left to right, that said i'd still avoid it because it makes the useCallback(...args) arity arbitrary as a consequence and thus the implementation details probably be more involved, something i've been trying to avoid on potentially hot public APIs, createElement/h are exceptions.

In the case of pinning them to the tail, it's ergonomically not possible if we still want useCallback(event => {}) to work as it would in React, that you can if required optionally extend to – useCallback((event, props, [a, b, c]) => {}, [a, b, c]) if avoiding inline-closures on each render is a goal.

@thysultan
Copy link
Member Author

thysultan commented Feb 21, 2019

Suspense

In conjunction with lazy and future useResource(planned) a general primitive heuristic(throwing promises) that any hook can use to interact/signal with a suspense boundary, and effectively re-create what would otherwise be possible with provided API's like useResource.

Exhibit A:

function Example (props) {
	const data = useResource('/details')

	return <pre>JSON.stringify(data)</pre>
}

function useResource (url) {
	const [data, dispatch] = useState(null)

	if (data === null) {
		throw fetch(url).then((res) => res.json()).then(payload => dispatch(payload))
	}

	return data
}

render(
<Suspense fallback="Loading...">
    <Indirection>
        <Example/>
    </Indirection>
</Suspense>, document)

At the moment you could probably implement this with error boundaries, but any error boundary between you and the consumer would be able to intercept this propagation. Special casing throwing promises/thenables as a special contract to talk to Suspense boundaries affords this channel of communication to any hook outside of provided API's like useResource(planned).

@Zolmeister
Copy link

Alright, I am in favor of passing the second argument of useCallback (which should be an array) as the first argument of the call if the argument exists.

I like the idea. Can it be done without throwing? e.g. via hook?

@thysultan
Copy link
Member Author

Can it be done without throwing? e.g. via hook?

Maybe generators but would be more involved and go against the spirit of hooks.

@mcjazzyfunky Related to the key/ref props concerns we where discussing before – reactjs/rfcs#107.

@mcjazzyfunky
Copy link
Collaborator

mcjazzyfunky commented Feb 22, 2019

@thysultan Many thanks for that React RFC link - indeed really interesting (seems like you have some kind of fortunetelling crystal ball 😄)
Regarding that generator stuff: Like always, one major problem would be that TypeScript and Flow do not support type inference for this generator use case yet.

@mcjazzyfunky
Copy link
Collaborator

mcjazzyfunky commented Mar 3, 2019

@thysultan When I saw Dyo's way of handling context and error boundaries with hooks const [theme, setTheme] = useContext(ThemeCtx) and useBoundary(exception => ....) for the first time, I thought "Mmmh, that's a nice idea".
Then today, I have read the following article by Dan Abramov:

https://overreacted.io/why-isnt-x-a-hook

You find the following line there:

[...] something that doesn't work well as a Hook [...] - for example useProvider(), useCatch() [...]

Actually Dan has a good point.
Indeed, with Dyo's useContext and useBoundary hooks it's possible to implement custom hooks that will not necessarily compose well in 100% of all cases, while in React, hooks always compose well.
Any opinions?

@thysultan
Copy link
Member Author

@mcjazzyfunky I disagree.

Conceptually useBoundary is exactly like useEffect, and useContext is like useContext in React with the semantics of useState(the ability to update).

That is to say useBoundary and useContext in Dyo are as much compose-able to useEffect, useContext and useState/useReducer found in React.

I would like to be proven wrong though if you have a case where that is true for useBoundary/useContext vs what is possible with useEffect/useState.

@mcjazzyfunky
Copy link
Collaborator

Just assume you have the following code snippet (where useYellowTheme does exactly what its name says):

function Outer(props) {
  useYellowTheme()
  useSomeMysteriousCustomHook()

  return h('div', null, props.children)
}

function Inner() {
  const [theme] = useContext(ThemeCtx)
  
  return `Used theme color: ${theme}`
}

render(h(Outer, null, h(Inner)), document.getElementById('app'))

Then you have a look at the output and you see:

Used theme color: red

This is not what you would expect.
The reason, of course, is that the custom hook function useMysteriousCustomHook does some evil stuff.
So useYellowTheme and useMysteriousCustomHook are NOT composable/combinable.
I don't think something like that would not be possible in React (at least not without using global state or other evil stuff).

Here is the full demo:
https://jsfiddle.net/brsm0jhw
(@thysultan, btw, please have a look at the Javascirpt console - is that infinite recursion some bug in Dyo?)

@thysultan
Copy link
Member Author

thysultan commented Mar 3, 2019

This is not what you would expect.

What you would except depends on different factors.

Firstly if we assume that useYellowTheme and useSomeMysteriousCustomHook are external dependencies, then they would not have access to ThemeCtx to be able to affect them. If we double down on that and assume that they create their own context providers then we can't assume anything or have any basis for what to expect, similar to custom hooks that use useState/useEffect.

function Foo (props) {
const value = useCustomValue(1)

return h('div', null, value)
}

In which case value could be anything and useCustomValue could trigger as many updates as it would like with useEffect/useLayout etc.

So for either useYellowTheme or useSomeMysteriousCustomHook to be able to affect a context provider downstream they would need a reference to it provided by the consumer, in which case we can clearly identify the transfer of control.

Now instead assuming Inner is our consumer.

Today in React any component with a reference to the provider above it could alter the value of the theme.

function Outer(props) {
  useYellowTheme()
  useSomeMysteriousCustomHook()

  return h('div', null, h(ThemeCtx, {value: "red"}, props.children))
}

render(h(Outer, null, h(Inner)), target)

So in that case we can expect something different as well, assuming the base assumption was that the value should be green. The parallels as identical.

btw, please have a look at the Javascirpt console - is that infinite recursion some bug in Dyo?

Yes that was a bug, published 0.0.22 – https://unpkg.com/[email protected] which i assume will eventually propagate to https://unpkg.com/dyo as well.

@mcjazzyfunky
Copy link
Collaborator

@thysultan Maybe I am wrong, but I just cannot think of a case where two well-designed (= without any evil stuff/unexpectable side-effects) custom hooks in React are not composable. With Dyo, if you have some useLightTheme and useDarkTheme custom hooks, both as public API, they are obviously NOT composable. That's basically all I wanted to point out (Dyo seems a bit more powerful here, and with power comes responsibility ...).
Okay, thanks for your detailed answer, I think I got your point.

@thysultan
Copy link
Member Author

thysultan commented Mar 3, 2019

(>= without any evil stuff/unexpectable side-effects) custom hooks in React are not composable... With Dyo, if you have some useLightTheme and useDarkTheme custom hooks, both as public API, they are obviously NOT composable.

If it's possible in React it should be similarly be possible in Dyo and vice-versa, as the API contract is in-parallel to useState(useContext) and useEffect(useBoundary).

(Dyo seems a bit more powerful here, and with power comes responsibility ...).

You could still archive the same with React useContext albeit with the need for an extra "provider" component, something useContext and hooks in general are meant to alleviate to begin with, so i'm not sure why they didn't go with this direction from the get-go. Maybe that is what Context.write attempts to provide.

@Zolmeister
Copy link

My interpretation is that you would never use useSuspense/useBoundary within another hook, therefore it shouldn't be a hook itself. It's not that crazy; you could imagine another interface that wasn't a hook:

function MyComponent(){}
MyComponent.componentDidCatch = _=> h('failed')
function MyComponent(){
  return componentDidCatch(h(Throws), _=> h('failed'))
}

@thysultan is there an example of a custom hook that would use useSuspense?

@thysultan
Copy link
Member Author

A useSuspense hook doesn't exist, only a Suspense component. The low level interface between interacting with a suspense root is around the heuristic of throwing a Promise, which are not caught by useBoundary boundaries and propagate to the nearest suspense root, this makes it possible to interact with a suspense root from any hook/event.

For example useResource could use this heuristic, by 1. throwing a promise when the cache is cold, 2. caching it after the promise is resolved and 3. returning the data synchronously when the cache is hot, given Dyo/React retries rendering the component when once the Promise resolves.

Dyo.lazy like React.lazy can also use this to throw a Promise if the cache of the component doesn't yet exist etc.

@Zolmeister
Copy link

I see
How about useBoundary? Can you give an example where it would make sense inside of another hook?
Could useBoundary be a component (similar to Suspense)?

@thysultan
Copy link
Member Author

The useBoundary hook services the needs of replacing componentDidCatch, for example you could have a custom hook useErrorReportingService.

const useErrorReportingService => useBoundary(exception => {
    fetch(`post/to/error-reporting-service/${JSON.stringify(exception)}`)
})

const Example = props => {
    useErrorReportingService()
    return props.children
}

@Zolmeister
Copy link

Not a very compelling example (the error reporting function is more correctly written purely, as below). Can you provide a non-trivial example? (One where the hook qualities provide value)

const Example = props => {
    useBoundary(errorReportingService)
    return props.children
}

Why not make it a component like Suspense?

@thysultan
Copy link
Member Author

How would you for example do the fore-mentioned with a Boundary component? Pass the callback as a props?

@Zolmeister
Copy link

Yes, like suspend <Boundary fallback="">

@thysultan
Copy link
Member Author

thysultan commented Mar 5, 2019

  1. What should fallback be, a callback(render prop), an element or any?
  2. Assuming fallback is an element and there's another <Boundary callback={() => {}}> prop, how would it pass the data to the fallback prop, that is when an author wants to report the error to a service then re-render the interface with the error report?

@Zolmeister
Copy link

  1. I figured it would be a callback matching didComponentCatch(error) which may or may not return an element
  2. see 1

@thysultan
Copy link
Member Author

thysultan commented Apr 8, 2019

Per latest commit, useBoundary has been replaced by a <Boundary fallback={() => {}}> component.

The fallback can be anything you can render (function/array/element/etc). When it is a function the exception object is passed to it.

<Boundary fallback={<h1>Error</h1>}>
<Boundary fallback={props => JSON.stringify(props)}>
<Boundary fallback={[props => fetch('report/...'), props => JSON.stringify(props)]}>

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