-
Notifications
You must be signed in to change notification settings - Fork 14
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
Webpack into native module #402
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small things need to get fixed.
def DEFAULT_MIN_SDK_VERSION = 19 | ||
def DEFAULT_TARGET_SDK_VERSION = 27 | ||
|
||
android { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the package name in here: namespace "app.edge.reactnative.currencyplugins"
, and then remove it from the Android Manifest.
Putting the namespace in the manifest works, but is deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is not how it is configured in accountbased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See edge-core-js for an example. I guess we need to update accountbased too.
android/src/main/AndroidManifest.xml
Outdated
@@ -0,0 +1,3 @@ | |||
<manifest xmlns:android="http://schemas.android.com/apk/res/android" | |||
package="app.edge.reactnative.currencyplugins"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the package=
prop once we add it to the build.grade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is not how it is configured in accountbased.
webpack.config.js
Outdated
console.log('adb reverse tcp:8101 tcp:8101') | ||
exec('adb reverse tcp:8101 tcp:8101', () => {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This port, 8101, the the one the GUI currently uses. That is fine!
However, edge-core-js uses 8080, edge-currency-accountbased used 8082, and edge-exchange plugins uses 8083. So it seems like 8081 would "fit" better than 8101.
webpack.config.js
Outdated
// Run `yarn start.plugins` to enable debug mode. | ||
// This mode will serve the plugin bundle via a local dev-server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This yarn instruction is incorrect.
plugins: [ | ||
new webpack.IgnorePlugin({ resourceRegExp: /^(https-proxy-agent)$/ }), | ||
new webpack.ProvidePlugin({ | ||
Buffer: ['buffer', 'Buffer'] | ||
}), | ||
new webpack.ProvidePlugin({ | ||
process: path.resolve('node_modules/process/browser.js') | ||
}) | ||
], | ||
resolve: { | ||
aliasFields: ['browser'], | ||
extensions: ['.ts', '.js'], | ||
fallback: { | ||
assert: require.resolve('assert'), | ||
crypto: require.resolve('crypto-browserify'), | ||
fs: false, | ||
http: require.resolve('stream-http'), | ||
https: require.resolve('https-browserify'), | ||
os: require.resolve('os-browserify/browser'), | ||
path: require.resolve('path-browserify'), | ||
stream: require.resolve('stream-browserify'), | ||
string_decoder: require.resolve('string_decoder'), | ||
url: require.resolve('url'), | ||
vm: require.resolve('vm-browserify') | ||
}, | ||
mainFields: ['browser', 'module', 'main'] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that this config file only needs to worry about edge-currency-plugins and not anybody else, I'm guessing we can really minimize this section. Does edge-currency-plugins really depend on all this Node.js stuff? Probably not all of it.
However, I understand that we might want to keep things stable for now. Maybe we can create a follow-on task and handle this cleanup in a future release, if stability is a concern.
README.md
Outdated
|
||
```js | ||
addEdgeCorePlugins({ | ||
ethereum: plugins.ethereum, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a bad example for this repo.
README.md
Outdated
``` | ||
|
||
If you want to debug this project, run `yarn start` to start a Webpack server, | ||
and then adjust your script URL to http://localhost:8101/edge-currency-plugins.js. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do adjust the port number, change it here too.
src/react-native.ts
Outdated
const { sourceUri } = EdgeCurrencyPluginsModule.getConstants() | ||
|
||
export const pluginUri = sourceUri | ||
export const debugUri = 'http://localhost:8101/edge-currency-plugins.js' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do change the port number, adjust it here too.
87f87dd
to
844b741
Compare
68285eb
to
11bbc8e
Compare
eb73f30
to
75c8e91
Compare
I'd much rather have a push strategy for this state, but the best I could achieve and test to work is a pulling strategy. The plugin factory will pull for the first instance.
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
none