Open
Conversation
|
Can this be merged? |
Author
|
Yes it can but it still have all |
danimoh
added a commit
to nimiq/ledger-api
that referenced
this pull request
Nov 13, 2020
readable-stream has circular dependencies which cause inheriting a class which is still undefined at that moment, resulting in a runtime error. readable-stream is imported by @ledgerhq/hw-app-btc/src/hashPublicKey > ripemd160 > hash-base. To fix the build, we replace it by stream which gets polyfilled by rollup-plugin-node-polyfills. Note that stream and readable-stream are largely compatible and effectively the same code. However, the stream polyfill used by rollup-plugin-node-polyfills is an older version which has less problems with circular dependencies. The circular dependencies are currently being resolved in readable-stream though and once merged (see nodejs/readable-stream#348) the replacement should be removed or even turned around. Note that without the replacement, the stream polyfill and readable-stream are both bundled, which is not desirable. Note that the stream polyfill / older readable-stream version also has circular dependencies but is able to run. Other options tried to resolve this issue which didn't help were: - Update readable-stream: the circular dependencies are still present in the current version of readable-stream. See nodejs/readable-stream#348 for progress on that issue getting resolved. - Update rollup-plugin-node-polyfills dependencies and polyfills: While the plugin is not being updated and maintained anymore, there has been a recent pull request with updates: ionic-team/rollup-plugin-node-polyfills#14. This didn't help though. Other options which have not been tried: - Using @rollup/plugin-commonjs option dynamicRequireTargets: replicates the logic for dynamic requires and might fix problems with circular dynamic requires. - rewrite imports as in rollup/rollup#1507 (comment): This is in the end similar to what we did. Note that also re-ordering of the plugin order mitigated the issue of builds that fail to run, however, still readable-stream has more circular dependencies than the stream polyfill and therefore we keep this fix for now. The re-ordering of plugins will be re-visited in a later change / commit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi,
Here is a simple pull request to update dependencies of the project. This pull request includes:
testcommand of the project)rollup-plugin-commonjs-->@rollup/plugin-commonjsrollup-plugin-json-->@rollup/plugin-jsonrollup-plugin-node-resolve-->@rollup/plugin-node-resolveWARNING: I tried to fix everything, but a module (
browserify-fs) includes a lot of dependencies with security problems (3 included 1 with high danger). I didn't find any alternative, so any feedback about it is welcome.