search

nodejs / readable-stream

Node-core streams for userland
https://nodejs.org/api/stream.html
Other
1.03k stars 228 forks source link

3.6.1 breaks ES5 compatibility #507

Closed MonsieurLanza closed 1 year ago

MonsieurLanza commented 1 year ago

Our ES5 build breaks when readable-stream gets updated to 3.6.1.

This commit precisely introduces ES6 syntax, such as const keywords, class methods shorthands…

https://github.com/nodejs/readable-stream/commit/fec7fe9869a1f46bee0ce1603538059b780fe25a

I am aware that ES5 is a somewhat old thing, however, in my understanding, an EcmaScript version bump should not occur in a patch version.

kanongil commented 1 year ago

Indeed. Either that or the support section needs an update… https://github.com/nodejs/readable-stream/blob/24b22f69f7b31eab1d24a3b8b136cf88f9cc7a38/README.md?plain=1#L27

mcollina commented 1 year ago

Sorry about this, it was definitely not the goal. Could you send me a small script to validate that I don't break it again?

ningyougan commented 1 year ago

hi,

you can use es-check to check your lib can be use by es5.

mcollina commented 1 year ago

@ningyougan @kanongil @MonsieurLanza could you verify if https://github.com/nodejs/readable-stream/pull/508 fixes it?

ningyougan commented 1 year ago

I change your new commit pkg in node_modules ,but oops

This dependency was not found:

* abort-controller in ./node_modules/browserify-sign/node_modules/readable-stream/lib/internal/streams/pipeline.js, ./node_modules/hash-base/node_modules/readable-stream/lib/internal/streams/pipeline.js

To install it, you can run: npm install --save abort-controller

I compare #508 with 3.6.1 ,it's a new external dep. Is aboort-controller necessary?

Thx for your congtribution! Maybe this bug is my bad, plz help me to confirm the question.

mcollina commented 1 year ago

@ningyougan you would need to run this in an environment that has either a global AbortController or install abort-controller as a dependency.