Skip to content

Commit 21ed048

Browse files
committed
rollup: replace problematic bitcoin dependency readable-stream
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.
1 parent 97736f2 commit 21ed048

File tree

1 file changed

+40
-7
lines changed

1 file changed

+40
-7
lines changed

rollup.config.js

+40-7
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,25 @@ function fixedEslint(options) {
4343
};
4444
}
4545

46+
// function debugModuleDependencies(module) {
47+
// return {
48+
// name: 'debug-module-dependencies',
49+
// writeBundle() {
50+
// console.log(`\n\nDebug module dependencies for ${module}.`);
51+
// const moduleIds = [...this.getModuleIds()].filter((id) => id.includes(module));
52+
// console.log('Matched bundled module ids:', moduleIds);
53+
// for (const moduleId of moduleIds) {
54+
// const moduleInfo = this.getModuleInfo(moduleId);
55+
// console.log('\n', {
56+
// id: moduleInfo.id,
57+
// importers: moduleInfo.importers,
58+
// dynamicImporters: moduleInfo.dynamicImporters,
59+
// });
60+
// }
61+
// },
62+
// };
63+
// }
64+
4665
// Hoist dependency imports for dynamic imports similar to rollup's dependency hoisting for regular imports
4766
// (see https://rollupjs.org/guide/en/#why-do-additional-imports-turn-up-in-my-entry-chunks-when-code-splitting) for
4867
// quicker loading of the dependencies. Note that rollup does not do this by default, as the execution order is not
@@ -126,6 +145,19 @@ export default (commandLineArgs) => {
126145
})),
127146
preserveEntrySignatures: 'allow-extension', // avoid rollup's additional facade chunk
128147
plugins: [
148+
alias({
149+
entries: {
150+
// replace readable-stream imported by @ledgerhq/hw-app-btc/src/hashPublicKey > ripemd160 >
151+
// hash-base by stream which gets polyfilled by rollup-plugin-node-polyfills. Note that stream and
152+
// readable-stream are largely compatible and effectively the same code. However, the stream
153+
// polyfill used by rollup-plugin-node-polyfills is an older version which has less problems with
154+
// circular dependencies. The circular dependencies are currently being resolved in readable-stream
155+
// though and once merged (see https://github.com/nodejs/readable-stream/issues/348), this alias
156+
// should be removed or even turned around. Note that without the replacement, the stream polyfill
157+
// and readable-stream are both bundled, which is not desirable.
158+
'readable-stream': 'stream',
159+
},
160+
}),
129161
fixedEslint({
130162
throwOnError: isProduction,
131163
}),
@@ -153,6 +185,7 @@ export default (commandLineArgs) => {
153185
],
154186
}),
155187
hoistDynamicImportDependencies(),
188+
// debugModuleDependencies('stream'),
156189
],
157190
watch: {
158191
clearScreen: false,
@@ -200,20 +233,20 @@ export default (commandLineArgs) => {
200233
sourcemapPathTransform,
201234
},
202235
plugins: [
203-
fixedEslint({
204-
throwOnError: isProduction,
205-
}),
206-
typescript({
207-
include: ['src/demo/**', 'src/lib/**'],
208-
noEmitOnError: isProduction,
209-
}),
210236
// typescript needs the import as specified to find the .d.ts file but for actual import we need .es.js file
211237
alias({
212238
entries: {
213239
'../../dist/low-level-api/low-level-api': '../low-level-api/low-level-api.es.js',
214240
'../../dist/high-level-api/ledger-api': '../high-level-api/ledger-api.es.js',
215241
},
216242
}),
243+
fixedEslint({
244+
throwOnError: isProduction,
245+
}),
246+
typescript({
247+
include: ['src/demo/**', 'src/lib/**'],
248+
noEmitOnError: isProduction,
249+
}),
217250
resolve({
218251
preferBuiltins: true, // don't touch imports of node builtins as these will be handled by nodePolyfills
219252
}),

0 commit comments

Comments
 (0)