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

Add Web Support #45

Closed
wants to merge 10 commits into from
Closed

Conversation

PhysicsAreBad
Copy link

Addressing suggestion in #11 with localStorage support.

@mrousavy
Copy link
Owner

mrousavy commented Apr 2, 2021

Hi! Thanks for the PR! I've never used RN Web before - how do I test this out? Could you also add a web app to the example/ so I can test it?

Copy link
Owner

@mrousavy mrousavy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments, once those are fixed and I have a react-native-web example in the example/ folder we can merge this 👍

src/index.web.ts Outdated Show resolved Hide resolved
src/index.web.ts Outdated Show resolved Hide resolved
src/index.web.ts Outdated Show resolved Hide resolved
@PhysicsAreBad
Copy link
Author

Made suggested changes @mrousavy

src/index.web.ts Outdated Show resolved Hide resolved
Copy link
Owner

@mrousavy mrousavy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay I have left a few nitpicking comments, I'd like them to be resolved (only affects example). After that, I think everything looks perfect 👌

example/src/Benchmarks.ts Outdated Show resolved Hide resolved
example/src/Benchmarks.ts Outdated Show resolved Hide resolved
example/tsconfig.json Outdated Show resolved Hide resolved
example/tslint.json Outdated Show resolved Hide resolved
@PhysicsAreBad
Copy link
Author

I made the further suggested edits.

@mrousavy
Copy link
Owner

mrousavy commented Apr 6, 2021

So I ran the App for iOS and Android and nothing seems to be broken there, so that's good. However, when I run yarn web I get the following error:

example $ yarn web
yarn run v1.22.10
$ cd web && webpack serve
ℹ 「wds」: Project is running at http://localhost:8080/
ℹ 「wds」: webpack output is served from /
ℹ 「wds」: Content not from webpack is served from /Users/mrousavy/Projects/react-native-mmkv-pr/example/web
✖ 「wdm」: asset vendor.js 1.33 MiB [emitted] (name: vendor) 1 related asset
asset app.js 387 KiB [emitted] (name: app) 1 related asset
asset index.html 392 bytes [emitted]
runtime modules 51.4 KiB 20 modules
cacheable modules 1.28 MiB
  modules by path ../node_modules/webpack-dev-server/ 21.1 KiB 11 modules
  modules by path ../node_modules/react-dom/ 901 KiB 6 modules
  modules by path ../node_modules/html-entities/lib/*.js 61 KiB 5 modules
  modules by path ../node_modules/querystring/*.js 4.51 KiB 3 modules
  modules by path ../node_modules/react/ 70.6 KiB 2 modules
  modules by path ../node_modules/webpack/hot/*.js 1.42 KiB
    ../node_modules/webpack/hot/emitter.js 75 bytes [built] [code generated]
    ../node_modules/webpack/hot/log.js 1.34 KiB [built] [code generated]
  modules by path ../node_modules/url/*.js 23.1 KiB
    ../node_modules/url/url.js 22.8 KiB [built] [code generated]
    ../node_modules/url/util.js 314 bytes [built] [code generated]
../node_modules/webpack/hot/ sync nonrecursive ^\.\/log$ 170 bytes [built] [code generated]

ERROR in app
Module not found: Error: path argument is not a string

ERROR in app
Module not found: Error: path argument is not a string

webpack 5.30.0 compiled with 2 errors in 4681 ms
ℹ 「wdm」: Failed to compile.

@PhysicsAreBad

@mrousavy
Copy link
Owner

mrousavy commented Apr 6, 2021

Actually the native app also breaks by this PR. This must be some issue with the new babel config:

yarn run v1.22.10
$ react-native start --reset-cache
                                                      
                        #######                       
                   ################                   
                #########     #########               
            #########             ##########          
        #########        ######        #########      
       ##########################################     
      #####      #####################       #####    
      #####          ##############          #####    
      #####    ###       ######       ###    #####    
      #####    #######            #######    #####    
      #####    ###########    ###########    #####    
      #####    ##########################    #####    
      #####    ##########################    #####    
      #####      ######################     ######    
       ######        #############        #######     
         #########        ####       #########        
              #########          #########            
                  ######### #########                 
                       #########                      
                                                      
                                                      
warning: the transform cache was reset.
                    Welcome to Metro!
              Fast - Scalable - Integrated


transform[stderr]: Could not resolve "/Users/mrousavy/Projects/react-native-mmkv-pr/src/index" in file /Users/mrousavy/Projects/react-native-mmkv-pr/example/src/App.tsx.
transform[stderr]: Could not resolve "/Users/mrousavy/Projects/react-native-mmkv-pr/src/index" in file /Users/mrousavy/Projects/react-native-mmkv-pr/example/src/Benchmarks.ts.
 BUNDLE  ./index.tsx 

error: node_modules/react-native/Libraries/Components/DatePicker/DatePickerIOS.ios.js: /Users/mrousavy/Projects/react-native-mmkv-pr/example/node_modules/react-native/Libraries/Components/DatePicker/DatePickerIOS.ios.js: Missing class properties transform.
  114 |  */
  115 | class DatePickerIOS extends React.Component<Props> {
> 116 |   static DefaultProps: {|mode: $TEMPORARY$string<'datetime'>|} = {
      |   ^
  117 |     mode: 'datetime',
  118 |   };
  119 |
 BUNDLE  ./index.tsx 

error: node_modules/react-native/Libraries/Components/MaskedView/MaskedViewIOS.ios.js: /Users/mrousavy/Projects/react-native-mmkv-pr/example/node_modules/react-native/Libraries/Components/MaskedView/MaskedViewIOS.ios.js: Missing class properties transform.
  64 |  */
  65 | class MaskedViewIOS extends React.Component<Props> {
> 66 |   _hasWarnedInvalidRenderMask = false;
     |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  67 |
  68 |   render(): React.Node {
  69 |     const {maskElement, children, ...otherViewProps} = this.props;

A quick stackoverflow search showed me a possible approach, however I don't think that is a solution but rather a wacky workaround. Can we make the babel config only apply to the web project and leave the normal config for the native apps as is? Surely react-native-web has a solution for this.

Copy link
Owner

@mrousavy mrousavy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my 2 comments above.

  1. Can't run yarn web to see the app.
  2. Native example app breaks because of the babel config changes.

@@ -112,6 +113,9 @@
"useTabs": false
}
]
},
"globals": {
"localStorage": true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially better to add /* global localStorage */ to the top of src/index.web.ts, since it's only available as a global on the web platform, and not to the rest of the code...

ref: https://eslint.org/docs/rules/no-undef

@@ -53,6 +53,7 @@
"@react-native-community/eslint-config": "^2.0.0",
"@release-it/conventional-changelog": "^2.0.0",
"@types/jest": "^26.0.0",
"@types/node": "^14.14.37",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I'm missing something obvious, but I cannot find where these types are used?

Types in the @types/node package are only available when running inside Node.js, not in the browser which this new code is targeting, right?

@nandorojo
Copy link

I think it would be easier to have a separate example folder for web, and just use a minimal expo web template.

@mrousavy
Copy link
Owner

RN MMKV now supports web 🎉

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

Successfully merging this pull request may close these issues.

5 participants