search

jspm / jspm-core

JSPM Core libraries including platform builtin shims
https://jspm.org
Other
51 stars 13 forks source link

Upgrade to readable-stream (?) #95

Open relu91 opened 6 months ago

relu91 commented 6 months ago

Hi all, thank you for bundling together node polyfils for browsers. I'm using esbuild-plugin-polyfill-node and I'm using streams. I was wondering if upgrading the bundled browserify streams to official readable stream implementation is possible.

Browserify readable stream is stuck into 2020 and it does not have support for Readable.from function (because it refers to an old version of readable-stream package). The lastest version of readable-stream (v4.x.x) supports browsers and it is still actively maintained. I've already tried it in a bunch of projects with no side-effects.

guybedford commented 6 months ago

I would be happy to update the readable stream polyfill implementation used in this project to a newer one. Do you have any suggestions what a good option might be at this point?

relu91 commented 6 months ago

Thank you for considering this option! As far I can tell https://github.com/nodejs/readable-stream?tab=readme-ov-file#readable-stream v4.x.y is already a good candidate for polyfill. I'm trying to bake a PR but it seems that just removing stream-browserify and add the new package cause the export test to fail. As far as I can tell it seems that it is not exporting anything.

If you don't mind I can open a draft PR and we can move the discussion there.

guybedford commented 1 month ago

I finally started on a work-in-progress PR here in #101, hopefully should be able to get the update completed next weekend.

relu91 commented 1 month ago

Hi! Thank you for starting to work on this. As you might already have seen in the commit linked above I fixed it in my fork but I wasn't sure to bake a PR because of this change: https://github.com/relu91/jspm-core/commit/2a93d5c0742f22e7ad0779958e3847a6c9116111#diff-47407fecafdf5f5cd55403c3de457833ddf9b6fab45253c04e1dc4c7cb4495b1R28

If I recall correctly the readable stream has a kind of unfortunate naming of some node module (although valid for Nodejs resolve strategy) that got your jspmPlugin confused. Basically, there is a file named stream.js but there is also a folder called stream. I let you decide if it is something to fix in jspmPlugin or leave the workaround above.

I hope it helps!