Fix: run effect sync

[julien-f]

Fixes #48

Change done quickly in the browser, please test it thoroughly :)

---

The maintainers of this repo require that all pull request submitters adhere to the following:

• [x] I have read and will comply with the contribution guidelines. (required)
• [x] I have read and will comply with the code of conduct. (required)
• [x] All related documentation has been updated to reflect the changes made. (required)
• [x] My commit messages are cleaned up and ready to merge. (required)

The maintainers of this repository require you to select the semantic version type that the changes in this pull request represent. Please select one of the following:

• [x] major
• [ ] minor
• [ ] patch
• [ ] documentation only

[julien-f]

Running the effect synchronously is necessary when handling events:

• event.preventDefault()
• event.target.value: even persisting the event is not enough because the value of the target will be reset (by React) before accessing it

[Jascha-Sundaresan]

@divmain mentions in #48 that this "would result in an actual throw, vs a rejected promise - and, in turn, that could complicate effect composition somewhat."

is there a reason you didn't go with his proposed solution of

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

instead?

[julien-f]

Hey @Jascha-Sundaresan,

That's not necessary, Promise.resolve(fn()) can indeed throw before being able to return a promise but that's not the case for new Promise(resolve => resolve(fn()) 😄

Example.

[julien-f]

I there anything I can do to help this code to be merged?

[ryan-roemer]

@julien-f -- @divmain and I chatted about this on Friday and he is looking into it / the best solution (which I think may still involve the try/catch variant so that the end users don't have to try/catch wrap calls). At any rate, I'll leave it to Dale from here...

[julien-f]

@ryan-roemer As I explained in a previous comment, the try/catch variant does not do anything more, new Promise(resolve => resolve(fn())) already catch any exceptions thrown by fn and return a rejected promise 😄

[ryan-roemer]

Oh, haha, yeah that's totally right.

[Jascha-Sundaresan]

any word on this @divmain?

[julien-f]

Friendly poke ;)

[stefvhuynh]

@ryan-roemer, @divmain, this LGTM.

[stefvhuynh]

@divmain, anything you want to add?

[ryan-roemer]

maintainerd seems to be on the fritz again. I'm going to close and immediately re-open this PR to see if that kicks it.

[maintainerd[bot]]

maintainerd logging is enabled for this repository.

All actions related to rules and their enforcement will be logged here as a permanent record.

---

Click to view log...

- `2017-11-27T21:53:59.109Z:4dbc3f4`: The pull request was created - `2017-11-28T10:27:57.568Z:b5da625`: @julien-f checked `I have read and will comply with the [contribution guidelines](https://github.com/FormidableLabs/freactal/blob/master/CONTRIBUTE.md).`. - `2017-11-28T10:28:57.516Z:b5da625`: @julien-f checked `I have read and will comply with the [code of conduct](https://github.com/FormidableLabs/freactal/blob/master/CONTRIBUTE.md).`. - `2017-11-28T10:29:08.534Z:b5da625`: @julien-f selected `major` as the semantic version. - `2017-11-28T10:41:13.598Z:42b94fb`: @julien-f checked `All related documentation has been updated to reflect the changes made. _(required)_`. - `2017-11-28T10:41:17.370Z:42b94fb`: @julien-f checked `My commit messages are cleaned up and ready to merge. _(required)_`.

[ryan-roemer]

@julien-f -- maintainerd has failed because you need to format the original message opening this PR specifically. (Faiure is body - 4 checklist items remaining.)

Can you look at closed PRs like: https://github.com/FormidableLabs/freactal/pull/76 as there's some mandatory stuff there (like semver change etc) that enable this PR to go straight to release.

Thanks!

/cc @stefvhuynh @divmain

[ryan-roemer]

Also, looks like PR needs to catch up with master (?)

[divmain]

Hi @julien-f. Sorry to leave this idle. I'm catching up now on long-standing issues.

The solution you've proposed here doesn't address my concern from the issue. Errors will be swallowed into the abyss and not propagated down the promise chain (bad for effect composition) and potentially not reported in the console (bad generally). See the example here:

https://codepen.io/anon/pen/POddpy?editors=1111

The suggestion I made in the issue would suffice, and I may end up making that change myself. The other option would be a helper function that clones the event into a pseudo-event object that could be passed around asynchronously.

If it were just me, I would probably go in that direction, but I imagine others would prefer the synchronous one. So I think we should go with that.

[ryan-roemer]

@divmain -- Don't you need

resolve(throwErr())

instead of

resolve(throwErr)

in your example to be equivalent to .then(throwErr)? That does seem to be caught...

[divmain]

You're right! I'm basing this all on an old revision of the Promise spec, apparently. This being the case, I'm perfectly happy to merge this as-is.

[divmain]

This is technically a breaking change. @julien-f, I'll include this commit along with a couple of other breaking changes. However, please do indicate that you've accepted the contribution guidelines and code of conduct. Thanks, and sorry this has taken so long!

[julien-f]

@divmain I've updated the PR message 😄

It's indeed technically a breaking change but I don't really see how it might break anything in real life 😛

[divmain]

@julien-f, work on the other breaking changes got pushed back until January, so I'm merging this in now. Thanks again!

[divmain]

Published in v2.0.0.