Bundle Onyx with Webpack by kidroca · Pull Request #132 · Expensify/react-native-onyx

kidroca · 2022-05-16T17:34:39Z

Details

Bundle Onyx with Webpack to allow using Onyx outside of react-native

Using a test project with the follow package.json config

package.json

{
  "name": "test-onyx",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    "@testing-library/jest-dom": "^5.16.4",
    "@testing-library/react": "^13.2.0",
    "@testing-library/user-event": "^13.5.0",
    "expensify-common": "git+https://github.com/Expensify/expensify-common.git#2e5cff552cf132da90a3fb9756e6b4fb6ae7b40c",
    "localforage": "^1.10.0",
    "react": "^18.1.0",
    "react-dom": "^18.1.0",
    "react-native-onyx": "git+https://github.com/kidroca/react-native-onyx.git#11dc5443e4d6e7cac3a967344996466bb26966c8",
    "react-scripts": "5.0.1",
    "web-vitals": "^2.1.4"
  },
  "scripts": {
    "start": "react-scripts start"
  },
  "eslintConfig": {
    "extends": [
      "react-app",
      "react-app/jest"
    ]
  }
}

We can import and use Onyx in a non react-native project, like we do in App:

import Onyx from 'react-native-onyx';

Using the following package.json fields we tell bundlers to load different Onyx configurations per environment

browser -> Onyx is loaded for non react-native projects and uses localforage to manage IndexedDb
react-native -> Same as what we used to have, Onyx is loaded and uses the native storage
main -> Right now this field only aids IDEs with code completion from the unmodified source

Related Issues

Slack thread: https://expensify.slack.com/archives/C01GTK53T8Q/p1652389473489429

Automated Tests

No new functionality added
Existing tests are still pass

Linked PRs

kidroca

Some notes on the code

kidroca · 2022-05-16T17:42:44Z

package.json

  },
  "dependencies": {
    "ascii-table": "0.0.9",
-    "expensify-common": "git+https://github.com/Expensify/expensify-common.git#2e5cff552cf132da90a3fb9756e6b4fb6ae7b40c",


Specifying expensify-common under peer dependencies ensures both Expnesify/App and Expnsify/react-native-onyx use the same expensify-common version (the one from App)

kidroca · 2022-05-16T17:43:43Z

package.json

+    "expensify-common": ">=1",
    "localforage": "^1.10.0",
-    "react": "^17.0.2",
+    "react": ">=17.0.2",


>= so React 18 is accepted as well

kidroca · 2022-05-16T17:44:33Z

package.json

+    "@react-native-async-storage/async-storage": {
+      "optional": true
+    },
+    "localforage": {
+      "optional": true


async-storage and localforage are optional to let consumers use one or the other

kidroca · 2022-05-16T17:45:42Z

package.json

    ]
-  }
+  },
+  "sideEffects": false


This lets Webpack bundlers know that our code is not doing anything funky like change the Prototype or adding stuff to window and aids tree shaking

kidroca · 2022-05-16T17:47:13Z

webpack.common.js

+        /^expensify-common\/.+$/,
+        /^lodash\/.+$/,


Regex syntax is intentional since we import subpaths of these modules

lodash/merge

expensify-common/str/...

If we don't use a pattern the whole package is bundled even if it's a peer dependency: https://webpack.js.org/guides/author-libraries/#external-limitations

Cool, I was reading some about this and wanted to look into it further. Thanks for adding it.

kidroca · 2022-05-16T17:48:56Z

webpack.common.js

+        ..._.keys(pkg.peerDependencies),
+        ..._.keys(pkg.dependencies),


Our package dependencies are installed automatically into the consumer project node_modules

peerDependencies would be installed as well, or flagged as not met depending on the npm version
These are relatively easy to resolve if not installed, because we'll get an error that a module cannot be found

kidroca · 2022-05-16T17:50:39Z

cc @tgolen

tgolen

Found out we need the main field

package.json

This peer dependency is needed locally when we run tests

After webpack changes some unit tests broke due to mock resolution Because we're using the `react-native` preset of jest this file extension is .native.js. Otherwise, since jest prefers index.native.js over index.js it'll skip loading the mock

These presets are already covered by `metro-react-native-babel-preset` We should either use the metro preset or compose react/env/flow ourselves but not both

We don't have to lint bundled code in dist Since now we have .web and .native indexes we need to tell eslint that it's valid to import `./storage` even thought there's no `index.js`, because there's `index.native.js` and `index.web.js`

This should aide debugging and inspecting the package `module: index.js` might be able to help to import Onyx and performing changes in `node_modules` without having to re-build Onyx

The typings aid code completion in IDEs (and also allow Onyx to be used in Typescript project)

kidroca · 2022-05-17T13:36:54Z

✅ Fixed unit tests
This was a bit tricky to tackle. It turned out that since we're using the react-native preset for jest, jest was configured to resolve .native.js extensions like index.native.js and it would prefer them over plain index.js, so the mock for Storage wasn't getting picked up

✅ Fixed eslint errors
Similar problems with letting eslint know that there's no storage/index.js but storage/index.native.js and storage/index.web.js
Also ignored the dist directory since it's minified code

✅ Configured exports correctly
Looks like it's preferable to specify paths as ./dist/index.native.js instead of just dist/index.native.js so that IDEs don't complain

An IDE complaining the path react-native-onyx/web is not installed

✅ Added production grade source maps
These help with stack traces and debugging Onyx sources through parent projects
Now that the code is minified we need these to trace calls like Onyx.merge from App

✅ Added script that generates typescript typings (all the .d.ts files)
Again since code is now bundled the information about method signature cannot be resolved like it used to
In order to let IDEs continue to provide code completion we need to provide static typing of the library

Code completion is suggested for both react-native-onyx and react-native-onyx/web

✅ Verified all envs and entry points work in App and for new projects using Onyx
The package.json structure and exports/main are correctly resolved in

webpack (web/desktop)
metro (ios/android)

These help with stack traces and debugging Onyx sources through parent projects Now that the code is minified we need these to trace calls like `Onyx.merge` from App

kidroca · 2022-05-17T13:45:10Z

babel.config.js

+    presets: [
+        require('metro-react-native-babel-preset'),
+    ],
+    plugins: [
+        '@babel/plugin-proposal-class-properties',
+    ],


@tgolen I've found out the presets (react/env/flow) are all covered by metro-react-native-babel-preset, maybe we don't need any changes here, but I left the plugin

Ah, OK. Good to know. The plugin is required, yeah. Thanks!

kidroca · 2022-05-18T22:54:08Z

webpack.config.js

+module.exports = [
+    webConfig,
+    webDevConfig,
+    nativeConfig,
+    nativeDevConfig,
+];


Exporting an array of configs let's webpack build them in parallel

kidroca · 2022-05-18T23:10:17Z

I've found a way to generate .development.js bundles with content close to the original source, but that alone didn't provide code completion in my IDE

Not entirely sure why I don't have code completion - even when I change the import to react-native-onyx/lib/Onyx I don't get intellisense for Onyx methods
The only thing that restores code suggestion is

having .d.ts files and export.types
or reverting to the unbundled Onyx version

I'm not certain if the .development.js scripts would help us much

they still contain a lot of tooling boilerplate
if you want to edit Onyx from node_modules you have to remember to edit the .development.js script and not lib/**.

One thing that we might do is

"exports": {
    ".": {
      "development": "./index.js",
      "types": "./dist/types/index.d.ts",
      "default": "./dist/native.min.js"
    }

which should load from raw source, but only when we import react-native-onyx (e.g. only in App and other RN projects)

Can you review the new code and tell me what you think

single webpack.config.js file producing a list of 4 configs
.development.js scripts in dist

tgolen

The changes look good to me. I didn't know that about webpack being able to build the configs in parallel. Neat!

I think the last thing is just adding a few comments and instructions so that it's clear what everything is for, and then this should be good to merge.

tgolen · 2022-05-19T14:47:20Z

package.json

+  "main": "./dist/native.min.js",
+  "module": "./index.js",
+  "exports": {
+    "./web": {


Could you add a small section in the README about how to import this lib in a web-only application and maybe a quick description/example of using the development bundle (and why someone would want to use it)?

tgolen · 2022-05-19T14:48:13Z

webpack.config.js

+    },
+});
+
+const webDevConfig = merge(webConfig, {


Could you add a quick comment here explaining why there is a dev config (same for the native dev config)?

Onyx can be import as `react-native-onyx` in both native and web projects thanks to `react-native` and `browser` fields in `package.json` The `main` filed serves to provider pure source which satisfies code completion in IDEs, this renders .d.ts files unnecessary

kidroca · 2022-05-19T19:12:48Z

Hey @tgolen, while writing the documentation I was testing stuff and found out how to get rid of the .d.ts files, and also use the unmodified source for Onyx for react-native projects
Now the native source doesn't have to be bundled, which allows us to tweak it (experiment) from parent projects

Another change is we no longer have to import react-native-onyx/web for web projects, package.json has the following fields

react-native - react native cli would load package source from whatever path is added here
main - IDEs and other tools would load source from here (in our case it provides code completion for IDEs)
browser - bundlers like Webpack would prefer to load code from here

Using the browser field, bundlers would load the source from dist/web

Sorry for the new changes, but I think this makes the setup simpler and closer to what we previously had for react-native
(I've triple tested these changes, but would be happy if someone else confirms the changes are good)

tgolen

Awesome, I like that better. Cleans it up some!

README.md

tgolen · 2022-05-19T19:23:54Z

README.md

+Bundlers like Webpack respect that field and import code from the path resolved there
+
+```javascript
+import Onyx, { withOnyx } from 'react-native-onyx/web';


Suggested change

import Onyx, { withOnyx } from 'react-native-onyx/web';

import Onyx, { withOnyx } from 'react-native-onyx';

Co-authored-by: Tim Golen <tgolen@gmail.com>

README.md

tgolen · 2022-10-11T07:01:50Z

I agree with you that the only real reason it's helpful is for messing with Onyx code directly (strictly a time-saver). I don't think we need to spend too much time trying to provide that functionality, but I'm curious about the development.js idea and see how that works.

…

On Wed, May 18, 2022 at 10:01 AM Peter Velkov ***@***.***> wrote: Removed .d.ts.map files Will experiment with the .development.js idea and post back — Reply to this email directly, view it on GitHub <#132 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMABZX7WF2IQWFJRJ5Z7DVKUH4PANCNFSM5WCHB3FA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

kidroca · 2022-10-12T08:44:03Z

I'm curious about the development.js idea and see how that works.

Right now for web we directly return the minified code from dist/
What we can do is add a new index script - web.js and load that instead
Inside web.js would be a NODE_ENV check and depending on the value we load the minified code or the non minified code
Dev servers are running with NODE_ENV=development, while builds are configured for production

One thing to note is that we still load bundled code, so live changes to source code would still not be reflected - to make changes to Onyx live you'd have to modify the non-minified code from dist/ (which is still a lot easier compared to dealing with minified code)

If we want to save time on testing new thing in Onyx, perhaps we can make a script in App that

cd to node_modules/react-native-onyx
runs webpack in watch mode so any changes to lib are rebuild and sent to dist
now make changes to node_modules/react-native-onyx/lib source and see them live in web/desktop

tgolen · 2022-10-12T15:46:41Z

So strange. I wrote that reply months ago and it only now posted to GH.

I do still think it's a problem worth fixing to make development on Onyx source code easier. We've been making a lot of performance changes, so it's been in active development lately.

I like your second idea since that would allow us to edit the source code directly. Could we try that?

tgolen · 2022-10-12T15:46:54Z

I think maybe we should open a new GH issue for that

kidroca · 2022-10-12T18:32:20Z

@tgolen

I like your second idea since that would allow us to edit the source code directly. Could we try that?
I think maybe we should open a new GH issue for that

Yep, we have almost everything in place, we just need to:

Add a watch script in Onyx, that runs webpack in watch mode
Add a test-onyx script in App. It goes to Onyx folder and install devDependencies so we can build and re-build the app with the build command (no-op if they are already installed), then trigger the watch script

puneetlath · 2022-11-16T18:45:57Z

@kidroca @tgolen @parasharrajat just FYI, it looks like this PR likely caused this bug: Expensify/App#10622

tgolen · 2022-11-16T21:18:02Z

Do you mean this PR specifically, or is there another? There are multiple attached to that issue.

kidroca · 2022-11-17T21:15:34Z

@puneetlath
This PR caused the web part of the bug, because we forgot to test capturing metrics after the change

capturing metrics on web was not supposed to work after the change, and we didn't thought of testing setting the flag ONYX_METRICS: true
capturing metrics on native continued to work until the introduction of Onyx.update method

The Android issue was because of the introduction of the Onyx.update method which (at the time) didn't return Promise but was added to the list of decorated methods (activated by env flag)
https://github.com/Expensify/react-native-onyx/pull/133/files#diff-146c57cda221f8f5ee5c808eee0c6692b02c4ac2b8818d0104896a04be78e78aR917

Initially decorateWithMetrics was implemented to be able to decorate any func - promise returning or not, but the code was slightly more complicated and I was asked to make it work only for promise returning (async) functions in order to simplify

I guess we should have added documentation about only working with async function to

the place where we decorate all the Onyx (async) functions
and on decorateWithMetrics function description

puneetlath · 2022-11-18T16:33:59Z

Ok good learnings! Anything we should do now?

kidroca · 2022-11-19T19:17:23Z

We can update method documentation and comments to mention that only Promise returning methods can be decorated

We can update Onyx metrics to work on Web - we only disabled them so that Onyx can be used in other non react-native projects, because it caused issues with bundling - instead of applying a working around - disabling metrics for web (via a no-op web file), we can fix bundling

tgolen and others added 10 commits May 13, 2022 15:29

Add platform specific files for storage engines

53cb40b

Add platform specific files for performance metrics

dd324a0

Add webpack for a build process

5586d90

Add separate webpack configs for each build type

8e8a164

Add aliases for platform specific files

489ef4a

Get builds running

2c3f085

Run both builds with a single build script

7631fb7

update readme

e5b4d65

Commit built files for testing

c690f14

Bundle Onyx with Webpack

11dc544

kidroca commented May 16, 2022

View reviewed changes

tgolen requested changes May 16, 2022

View reviewed changes

package.json Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

kidroca added 5 commits May 16, 2022 23:32

Use main instead of exports for .

b20014d

Delete leftover LICENSE.txts

b4b053d

Add expensify-common as dev dependency

ddcf412

This peer dependency is needed locally when we run tests

Fix unit tests

12da0e7

After webpack changes some unit tests broke due to mock resolution Because we're using the `react-native` preset of jest this file extension is .native.js. Otherwise, since jest prefers index.native.js over index.js it'll skip loading the mock

Remove unnecessary babel presets and packages

79881fd

These presets are already covered by `metro-react-native-babel-preset` We should either use the metro preset or compose react/env/flow ourselves but not both

kidroca force-pushed the kidroca/onyx-webpack branch from 327e387 to 79881fd Compare May 17, 2022 07:09

kidroca added 3 commits May 17, 2022 10:35

Fix eslint errors

8b3b572

We don't have to lint bundled code in dist Since now we have .web and .native indexes we need to tell eslint that it's valid to import `./storage` even thought there's no `index.js`, because there's `index.native.js` and `index.web.js`

Add Onyx sources to package

661e039

This should aide debugging and inspecting the package `module: index.js` might be able to help to import Onyx and performing changes in `node_modules` without having to re-build Onyx

Generate typescript typings

1ed8cb9

The typings aid code completion in IDEs (and also allow Onyx to be used in Typescript project)

kidroca requested a review from tgolen May 17, 2022 13:37

kidroca marked this pull request as ready for review May 17, 2022 13:41

kidroca requested a review from a team as a code owner May 17, 2022 13:41

melvin-bot bot requested review from techievivek and removed request for a team May 17, 2022 13:42

Add source maps

44d97f1

These help with stack traces and debugging Onyx sources through parent projects Now that the code is minified we need these to trace calls like `Onyx.merge` from App

kidroca commented May 17, 2022

View reviewed changes

kidroca commented May 18, 2022

View reviewed changes

kidroca requested a review from tgolen May 18, 2022 23:10

tgolen requested changes May 19, 2022

View reviewed changes

kidroca added 2 commits May 19, 2022 19:04

Add docs and comments around webpack bundling

791ac5e

kidroca requested a review from tgolen May 19, 2022 19:12

Fix usage in non react-native project docs

1c1101a

tgolen requested changes May 19, 2022

View reviewed changes

Fix typo in README.md

c1cf681

Co-authored-by: Tim Golen <tgolen@gmail.com>

kidroca commented May 19, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

Update README.md

988ca9e

tgolen approved these changes May 19, 2022

View reviewed changes

tgolen merged commit 5d715aa into Expensify:main May 19, 2022

tgolen mentioned this pull request May 19, 2022

Use new method for importing Onyx Expensify/k2-extension#101

Merged

luacmartins mentioned this pull request May 24, 2022

Error updating Onyx in App Expensify/App#9163

Closed

kidroca deleted the kidroca/onyx-webpack branch May 25, 2022 16:42

roryabraham mentioned this pull request Oct 11, 2022

Fix storage/index.native.js to remove Platform.select #197

Merged

Santhosh-Sellavel mentioned this pull request Nov 13, 2022

[HOLD for payment 2022-11-16] [$500] Dev - Activating metrics on crashes the app on Android - reported by @parasharrajat Expensify/App#10622

Closed

		..._.keys(pkg.peerDependencies),
		..._.keys(pkg.dependencies),

	import Onyx, { withOnyx } from 'react-native-onyx/web';
	import Onyx, { withOnyx } from 'react-native-onyx';

Conversation

kidroca commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Related Issues

Automated Tests

Linked PRs

Uh oh!

kidroca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidroca commented May 16, 2022

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kidroca commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidroca commented May 18, 2022

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidroca commented May 19, 2022

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tgolen commented Oct 11, 2022 via email

Uh oh!

kidroca commented Oct 12, 2022

Uh oh!

tgolen commented Oct 12, 2022

Uh oh!

tgolen commented Oct 12, 2022

Uh oh!

kidroca commented Oct 12, 2022

Uh oh!

puneetlath commented Nov 16, 2022

Uh oh!

tgolen commented Nov 16, 2022

Uh oh!

kidroca commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

puneetlath commented Nov 18, 2022

Uh oh!

kidroca commented Nov 19, 2022

Uh oh!

kidroca commented May 16, 2022 •

edited

Loading

kidroca commented May 17, 2022 •

edited

Loading

kidroca commented Nov 17, 2022 •

edited

Loading