nodejs / readable-stream

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

[Fix] babel's "loose mode" class transform enbrittles BufferList #428

Closed ljharb closed 4 years ago

ljharb commented 4 years ago
Object.defineProperty(Object.prototype, util.inspect.custom, {
  set(v) { throw 'this should not happen inside readable-stream'; }
});

With this change, I believe the output will use [[Define]] instead of [[Set]] for https://github.com/nodejs/node/blob/c101251a95cc82142bee4637f8db6cc360a06d82/lib/internal/streams/buffer_list.js#L167, and thus no longer fail when Object.prototype is modified.

ljharb commented 4 years ago

Seems like https://github.com/vweevers/readable-stream/commit/432d4b77439a8a825d7a330c2d16410e9ac15fbf is when it first was put into loose mode, in #299, due to #293.

Does readable-stream still support pre-ES5 environments? #344 implies that it does not.

mcollina commented 4 years ago

We support IE11 in readable-stream v3, while we still support pre-es5 environments in v2. This can land but not be backported.

mcollina commented 4 years ago

Can you also regenerate the code?

cd build
node build.js v10.19.0
ljharb commented 4 years ago

just v3 is fine; I’m mainly concerned with the version of this that’s vendored in core; that one doesn’t need to run Babel at all :-)

will update the build shortly

mcollina commented 4 years ago

cc @vweevers what do you think?

vweevers commented 4 years ago

👍

ljharb commented 4 years ago

yay! what are the next steps to get this into core?

mcollina commented 4 years ago

I think you'll have to wait for npm to do a release, and then update core to that. Or you can force a replacement directly.

ljharb commented 4 years ago

done in https://github.com/npm/cli/issues/824 and https://github.com/nodejs/node/pull/31977