REF: remove rn-nodeify in favor or metro bundler configuration #8086
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| process[p] = bProcess[p]; | ||
| } | ||
| } | ||
| global.process = {}; |
There was a problem hiding this comment.
Bug: Process Shim Initialization Overwrites Environment
The process shim's updated initialization creates an empty global.process object without process.env, and no longer merges properties from the process npm package into an existing process object. This leads to a TypeError when process.env.NODE_ENV is later set, and other process properties previously provided by the shim may also be missing.
Additional Locations (1)
There was a problem hiding this comment.
shim is only included in RN runtime, where there are no env vars anyway
| tls: require.resolve('react-native-tcp-socket'), | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Bug: Missing Polyfill for path Module Causes Runtime Errors
The Metro configuration is missing a polyfill for the path module in extraNodeModules. While path-browserify previously handled this, it was removed from dependencies without adding an equivalent mapping. This will cause runtime errors if any code or dependency tries to use the path module.
There was a problem hiding this comment.
apparently path module is not used?
There was a problem hiding this comment.
apparently path module is not used?
I checked the code and didn’t see any occurrences of the path being used. I also built and ran the app successfully, so removing path-browserify should be safe.
ojokne
left a comment
ojokne
left a comment
There was a problem hiding this comment.
LGTM
Non-blocking: path-browserify is still referenced in the following via their "react-native" / "browser" mappings.
These mappings may no longer be necessary, as the path module doesn’t appear to be used. Just noting this for awareness.
|
Wake the fuck up samurai, we have PRs to merge
[all PRs for @marcosrdz] https://github.com/BlueWallet/BlueWallet/pulls/review-requested/marcosrdz |
rn-nodeifyin favor of simple metro bundler configurationreact-native-cryptotocrypto-browserify, same thing except it now doesnt try to installreact-native-randombytespath-browserify,process(shimmed own minmal implementation)tests pass, but have to stay vigilant in case something is broken