Issues with nullified events

[markacola]

I am experiencing an issue when directly passing an effect to an element as an event handler.

It is to do with the empty https://github.com/FormidableLabs/freactal/blob/master/src/effects.js#L9 causing the effectFn to be defered to the next tick, after which React has nullified the event, making it impossible to extract the values purely via effects.

Changing:

Promise.resolve()
      .then(() => effectFn(effects, ...args))

to:

Promise.resolve(effectFn(effects, ...args))

Fixes the issue by calling effectFn immediately. I am not sure if this will negatively affect the timing of anything else.

I can see in the testing example in the readme (https://github.com/FormidableLabs/freactal/blob/master/README.md#L640) that a wrapper function is used in the render to immediately extract the value from the element. This works fine, however I have always avoided function declaration within render for performance reasons.

Feel free to close this if it isn't considered an issue, however it is something that I have run into.

[markacola]

Happy to submit a PR with these changes, just let me know.

[markacola]

Quick repro: https://codesandbox.io/embed/mwQo7vgR9

import React from 'react'
import { provideState, injectState } from 'freactal'

const withState = provideState({
  initialState: () => ({
    value: ''
  }),
  effects: {
    // Error: Cannot read property `value` of null
    handleChangeInput: (effects, { target: { value } }) => state => ({
      ...state,
      value
    })
  }
})

const Input = ({ effects: { handleChangeInput }, state: { value } }) => (
  <input onChange={handleChangeInput} value={value} />
)

const InputWithState = withState(injectState(Input))

render(<InputWithState/>, document.getElementById('root'))

[divmain]

Sorry to keep you waiting @markacola. There are easy work-arounds, of course, but this seems like a pretty reasonable use-case.

The only down-side I can think of is when the effectFn throws an error. Making the change would result in an actual throw, vs a rejected promise - and, in turn, that could complicate effect composition somewhat.

As an alternative, we could change the line you indicated to the following:

memo[effectKey] = (...args) => new Promise((resolve, reject) => {
  try {
    resolve(effectFn(effects, ...args));
  } catch (err) {
    reject(err);
  }
})
  .then(() => effectFn(effects, ...args))
  .then(applyReducer);

This would cause the effectFn to be invoked synchronously the way you want, and would avoid any issues with errors being thrown about. I'm not 100% sure how I feel about that, but I'm willing to consider it.

Thoughts?

[markacola]

I think this seems like the best way to go about it, although it adds a fair bit of noise.

[mjoslyn]

I'm just using a helper to forward the target, you could also use event.persist() to forward the whole event

const dispatchTarget = fn => ev => fn(ev.target);

<Input onChange={dispatchTarget(effects.onChangeUsername)} />

[markacola]

The issue with the dispatchTarget solution is that it creates a new function for every call, changing the props and causing a rerender of the Input.

[mjoslyn]

@markacola is right, this will cause a re-render of the input.

[Jascha-Sundaresan]

I am unaware of any downside to the solution proposed by @divmain

Is there anything preventing us from moving forward with this?