React Native bundle #114

jacques-blom · 2020-05-19T18:54:15Z

Fixes #99

Additions:

Add the ability to set/get a batch function, which defaults to ReactBatchedUpdates.
Add ReactBatchedUpdates files for Web and Native that are imported to set the batch function to the renderer-specific function.
Add a native target to Rollup, which resolves ReactBatchedUpdates.

Result:
This gives us out of the box support for React DOM and React Native, with the library user not needing to make any changes.

Notes:
I haven't changed the use of unstable_batchedUpdates in the tests. (In fact I haven't been able to get the suite to pass on master, which I'll have another go at later, and file an issue if I'm still stuck.) But it'd be great if the maintainers or anyone with more contexts on the tests could weigh in here. It seems like we should use unstable_batchedUpdates from react-test-renderer here.

In future, we could add a separate entry point (e.g. 'recoil/alternate-renderers') that doesn't set a batch function and doesn't depend on react-dom or react-native. We should be able to do this really easily because the infrastructure for setting the batching function to anything is available (and already exported in this PR).

Prior art for this PR:

React Redux: Alternate renderers entry point and get/set batch functions
@dustinsoftware's PR with a similar implementation 🙌

jacques-blom · 2020-05-19T18:58:53Z

package.json

@@ -37,6 +37,7 @@
    "install-peers-cli": "^2.2.0",
    "jest-cli": "^26.0.1",
    "prettier": "^2.0.5",
+    "react-native": "^0.62.2",


I've added RN as a dev dependency. I'm not actually sure what the right approach is here, because react-dom and react-native are optional dependencies (one of them is required).

A good approach might be to remove react-dom from the peer deps, too.

It should work fine to make react-dom as a devDependency for the test runner and leave out react-native entirely

rollup.config.js

jacques-blom · 2020-05-19T19:04:03Z

src/npm/index.js

@@ -1,7 +1,7 @@
 'use strict';

 if (process.env.NODE_ENV === 'production') {
-  module.exports = require('./recoil.production.js');
+  module.exports = require('./recoil.production');


Removed the explicit extension so that the React Native bundler (Metro) imports recoil.{mode}.native.js. If this is too terse, we can add an index.native.js entry file and a react-native key to the package.json.

jacques-blom · 2020-05-19T19:05:19Z

src/Recoil.js

@@ -76,4 +80,5 @@ module.exports = {

  // Other functions
  isRecoilValue,
+  setBatch,


The library user can call setBatch and pass a batching function exported by whatever React renderer they use. Due to the current setup (the lack of an alternate-renderers entrypoint), they'll still need react-dom as a dependency.

rollup.config.js

dustinsoftware · 2020-05-19T22:26:47Z

src/Recoil.js

@@ -29,6 +29,10 @@ export type {
  RecoilValueReadOnly,
 } from './core/Recoil_RecoilValue';

+const {unstable_batchedUpdates} = require('./util/ReactBatchedUpdates');
+const {setBatch} = require('./util/Recoil_batch');
+setBatch(unstable_batchedUpdates);


I like that this is exported; future-proofs the library a bit. We should probably add a test for this once they are passing.

src/util/Recoil_batch.js

davidmccabe · 2020-05-20T01:27:27Z

src/Recoil.js

@@ -29,6 +29,10 @@ export type {
  RecoilValueReadOnly,
 } from './core/Recoil_RecoilValue';

+const {unstable_batchedUpdates} = require('./util/ReactBatchedUpdates');


Do I understand correctly that this module will be resolved differently depending on the build, thus giving us a different default and not requiring setBatch to be called on either platform?

Correct. You'll either get unstable_batchedUpdates from React DOM or from React Native here depending on the platform. Then we pass that platform-specific batching function to setBatch.

davidmccabe · 2020-05-20T02:03:35Z

I'd love it if someone with experience in this area could weigh in on whether this or #108 is better long-term. What I want:

Users don't have to think about this
Users' only includes code for the appropriate platform
Users don't have to wait on us to use other renderers

hirbod · 2020-05-20T19:55:16Z

Will this or approch #108 also work with Expo on iOS and Android? Because there is no .native.js in Expo even though it is react-native.

jacques-blom · 2020-05-20T20:16:17Z

It will, @hirbod. Expo uses the same bundler as a bare React Native app, so will also resolve the .native.js bundle.

jacques-blom · 2020-05-20T20:20:32Z

I'd love it if someone with experience in this area could weigh in on whether this or #108 is better long-term. What I want:

Users don't have to think about this

Users' only includes code for the appropriate platform

Users don't have to wait on us to use other renderers

Hey @davidmccabe. Both approaches are very similar. The difference with this one is that you can, as a library user, set the batching function to anything you want. So custom renderers are supported here:

import {setBatch} from 'recoil'
import {unstable_batchedUpdates} from 'react-test-renderer'

setBatch(unstable_batchedUpdates)

To answer your specific questions:

You'll only have to think about it if you use a renderer other than React DOM or RN.
I'm 90% sure the library user's bundler (they'll most likely use Webpack or Metro) will only bundle code for the necessary platform. I'll double check using Webpack and Metro.
See above, and the exported setBatch function.

brentvatne · 2020-05-22T18:02:15Z

@hirbod - there is a .native.js extension in Expo, it uses the same platform extensions as bare React Native but also adds .expo.js to that, as @jacques-blom pointed out :)

stephanoparaskeva · 2020-06-17T17:04:00Z

Is there any reason this hasn't seen a merge?

jacques-blom · 2020-06-17T20:25:40Z

Howdy @davidmccabe. Mind having a look at my reply above when you have a chance, please? It'd be great to get RN support merged in soon.

Let me know if there is anything else I can do to help move this along.

jacques-blom · 2020-06-17T20:28:22Z

(or if you're able to, @drarmstr) 🙂

rdy · 2020-09-16T02:17:06Z

I also applied the change fixing the react-dom reference, as noted by @mondaychen. Everything seems to be working for me, I haven’t merged the latest changes here yet, but I can try.

My react-native test is on an out of tree platform so it’s a little different than testing against mobile.

jacques-blom · 2020-09-16T06:58:04Z

Thanks @mondaychen. I didn't hit the react-dom error because I had it in my RN project's dependencies. I removed it and then got the error.

The problem was that I didn't do a new publish of my package after I fixed this.

@jacques-blom/recoil@0.0.16 includes this fix, and it works.

facebook-github-bot

@mondaychen has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

davidmccabe · 2020-09-16T16:17:04Z

Thanks so much for putting in all the work on this!

Is there a way to tell when module replacement is occurring on React Native? That would allow us to suppress the duplicate key error message.

haswalt · 2020-09-16T17:25:29Z

@davidmccabe this might be related: redux-saga/redux-saga#1961 looks like redux-saga had the same issue

jacques-blom · 2020-09-16T18:02:16Z

Thanks so much for putting in all the work on this!

Is there a way to tell when module replacement is occurring on React Native? That would allow us to suppress the duplicate key error message.

Redux Saga seemed to have issues with sagas being registered multiple times, but I haven't experienced any issues related to Fast Refresh on RN, so a check should be good enough.

The only downside to this, as far as I understand it, is the user won't see the warning if they have Fast Refresh enabled and there are actually two atoms with the same key.

What do you think, @davidmccabe?

Also, I couldn't find a way to check whether or not Fast Refresh is enabled. Do you think you could reach out to the RN from your side? Or perhaps someone else in the thread has some context on this.

Unfortunately module.hot is always truthy in RN nowadays, so a !!module.hot check won't work.

mondaychen · 2020-09-16T19:58:33Z

So I'm merging this, with a few changes I made on internal tool

Make .native.js file content hidden in internal repo (because internal repo does not have react-native and build system was unhappy)
fix some comments & lints
reverted changes in expectationViolation, instead, use console.warn in Recoil_Node for now.

Let's keep looking for an elegant solution for the duplicated atom key issue triggered by hot reload.

drarmstr · 2020-09-16T20:11:06Z

Would someone here be able to help with a regression test for React Native so it doesn't accidentally break in a future release?

facebook-github-bot · 2020-09-16T21:19:57Z

@mondaychen merged this pull request in 3b1bbc1.

jacques-blom · 2020-09-17T09:05:29Z

Thanks so much for everyone's contributions to this! It's super awesome that we got it merged.

@drarmstr, I'll have another go at a regression test. 👍

What is the release process for Recoil, @mondaychen? Will this be manually released with other changes? And do you know roughly when the release will happen? Thanks.

mondaychen · 2020-09-19T16:02:49Z

@jacques-blom thank you for working on this! It's a big step for Recoil.
Yes, we currently do manual releases.
The next release we probably want to bump the version number to 0.1.0. We plan to use a couple weeks to get more tests, documentation, examples and the blog post ready for this milestone. Will keep you posted!

haswalt · 2020-09-19T22:44:02Z

@mondaychen any chance we could get a "next" release on npm? Running nightlies maybe?

mondaychen · 2020-09-21T17:25:18Z

@haswalt Good idea. I'll talk to team and keep you guys posted.

mondaychen · 2020-09-25T18:12:49Z

Hi everyone! FYI we've published a "nightly build" branch for testing purposes. If you want to try out recoil with react native before we release next version, try install the nightly branch with the "install git branch" feature of npm/yarn:

npm install https://github.com/facebookexperimental/Recoil.git#nightly

Or

yarn add https://github.com/facebookexperimental/Recoil.git#nightly

a-eid · 2020-10-14T07:41:32Z

@mondaychen hey, thank you for your work.
I was wondering when do you think RN will be officially supported ?

YOEL311 · 2020-10-20T20:49:11Z

Thank you for your wonderful work
When will ReactNative support be perfect?

please update us

krakzk · 2020-12-21T08:50:57Z

Have been using the 'recoil-react-native: 0.0.8-patch1" for development. It seems that the repo has been discarded maybe. Can we safely port to the nightly update or will there be any breaking changes?

haswalt · 2020-12-21T09:10:11Z

We've been using nightly for development on ReactNative for a little while now and haven't experienced any issues.

krakzk · 2020-12-21T09:20:59Z

@haswalt Have you used the library https://www.npmjs.com/package/recoil-react-native

haswalt · 2020-12-21T09:22:26Z

@krakzk no beacuse that package is really old, none official and there is ReactNative support built into Recoil

mondaychen · 2020-12-21T20:50:41Z

@krakzk @haswalt the latest version on npm (0.1.2) has react native support.

thegauravthakur · 2021-01-08T16:11:53Z

@mondaychen which package are you talking about?

krakzk · 2021-01-08T17:30:23Z

@thegauravthakur The recoil official package instead of any others. Add in package as "recoil" : "0.1.2"

Summary: Fixes facebookexperimental/Recoil#99 **Additions:** - Add the ability to set/get a batch function, which defaults to `ReactBatchedUpdates`. - Add `ReactBatchedUpdates` files for Web and Native that are imported to set the batch function to the renderer-specific function. - Add a `native` target to Rollup, which resolves `ReactBatchedUpdates`. **Result:** This gives us out of the box support for React DOM and React Native, with the library user not needing to make any changes. **Notes:** I haven't changed the use of `unstable_batchedUpdates` in the tests. (In fact I haven't been able to get the suite to pass on `master`, which I'll have another go at later, and file an issue if I'm still stuck.) But it'd be great if the maintainers or anyone with more contexts on the tests could weigh in here. It seems like we should use `unstable_batchedUpdates` from `react-test-renderer` here. In future, we could add a separate entry point (e.g. `'recoil/alternate-renderers'`) that doesn't set a batch function and doesn't depend on `react-dom` or `react-native`. We should be able to do this really easily because the infrastructure for setting the batching function to anything is available (and already exported in this PR). **Prior art for this PR:** - React Redux: [Alternate renderers entry point](https://github.com/reduxjs/react-redux/blob/master/src/alternate-renderers.js) and [get/set batch functions](https://github.com/reduxjs/react-redux/blob/master/src/utils/batch.js) - dustinsoftware's [PR](facebookexperimental/Recoil#108) with a similar implementation {emoji:1f64c} Pull Request resolved: facebookexperimental/Recoil#114 Reviewed By: drarmstr, davidmccabe Differential Revision: D23733076 Pulled By: mondaychen fbshipit-source-id: 948d45dd43b6e9ebac86ed6dd8433f3a1207c799

jacques-blom added 4 commits May 19, 2020 17:25

Adds batch wrapper for other renderers

f8923eb

Remove imports

d7ccbab

Adds native build

51f5666

Undo removal

696930d

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 19, 2020

jacques-blom commented May 19, 2020

View reviewed changes

rollup.config.js Outdated Show resolved Hide resolved

jacques-blom commented May 19, 2020

View reviewed changes

rollup.config.js Outdated Show resolved Hide resolved

jacques-blom commented May 19, 2020

View reviewed changes

jacques-blom mentioned this pull request May 19, 2020

React Native Support #99

Closed

dustinsoftware mentioned this pull request May 19, 2020

React native #108

Closed

dustinsoftware reviewed May 19, 2020

View reviewed changes

davidmccabe reviewed May 20, 2020

View reviewed changes

jacques-blom added 2 commits May 20, 2020 22:23

Remove RN as a dependency

cc1dff6

Change default function to throw error

17e3182

drarmstr added the enhancement New feature or request label May 23, 2020

jacques-blom added 4 commits June 17, 2020 21:17

Merge branch 'master' into react-native

148ec8d

Remove ReactDOM batch calls

155d017

Setup react batching in tests

09ab13f

Rebuild

8589b62

Merge remote-tracking branch 'upstream/master' into react-native

48e0d86

facebook-github-bot reviewed Sep 16, 2020

View reviewed changes

facebook-github-bot closed this in 3b1bbc1 Sep 16, 2020

facebook-github-bot added the Merged label Sep 16, 2020

mondaychen mentioned this pull request Nov 26, 2020

Fix React Native build #763

Closed

React Native bundle #114

React Native bundle #114

Conversation

jacques-blom commented May 19, 2020 • edited by mondaychen Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmccabe commented May 20, 2020

hirbod commented May 20, 2020

jacques-blom commented May 20, 2020

jacques-blom commented May 20, 2020 • edited Loading

brentvatne commented May 22, 2020 • edited Loading

stephanoparaskeva commented Jun 17, 2020

jacques-blom commented Jun 17, 2020 • edited Loading

jacques-blom commented Jun 17, 2020

rdy commented Sep 16, 2020 • edited Loading

jacques-blom commented Sep 16, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

davidmccabe commented Sep 16, 2020

haswalt commented Sep 16, 2020

jacques-blom commented Sep 16, 2020

mondaychen commented Sep 16, 2020

drarmstr commented Sep 16, 2020

facebook-github-bot commented Sep 16, 2020

jacques-blom commented Sep 17, 2020

mondaychen commented Sep 19, 2020

haswalt commented Sep 19, 2020

mondaychen commented Sep 21, 2020

mondaychen commented Sep 25, 2020

a-eid commented Oct 14, 2020

YOEL311 commented Oct 20, 2020 • edited Loading

krakzk commented Dec 21, 2020

haswalt commented Dec 21, 2020

krakzk commented Dec 21, 2020 • edited Loading

haswalt commented Dec 21, 2020

mondaychen commented Dec 21, 2020

thegauravthakur commented Jan 8, 2021

krakzk commented Jan 8, 2021

jacques-blom commented May 19, 2020 •

edited by mondaychen

Loading

jacques-blom commented May 20, 2020 •

edited

Loading

brentvatne commented May 22, 2020 •

edited

Loading

jacques-blom commented Jun 17, 2020 •

edited

Loading

rdy commented Sep 16, 2020 •

edited

Loading

YOEL311 commented Oct 20, 2020 •

edited

Loading

krakzk commented Dec 21, 2020 •

edited

Loading