search

nodejs / readable-stream

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

Removed numeric separator #491

Closed jaxoncreed closed 1 year ago

jaxoncreed commented 1 year ago

This is a simple pull request that removes the numeric separator from validators.js.

While the numeric separator is nicer to look at, it causes problems when this library is used in React-Native's default configuration. https://github.com/facebook/metro/issues/645. It is possible to change the default configuration manually, but that's overhead for a developer. To make it easier to install developer tooling that uses readable-stream it would be great to remove the numeric separators.

mcollina commented 1 year ago

Could you make this change inside the build/ scripts?

jaxoncreed commented 1 year ago

Ah I see. I've added it to the "replacements" file. Though, I'm unsure if I ran the build script properly as many files have minor style changes. Is there an instruction guide anywhere for running the build script?

mcollina commented 1 year ago

@ShogunPanda PTAL

ShogunPanda commented 1 year ago

Also looks good to me in regard of changes in the build folder. All other changes are mostly not needed.

@jaxoncreed can you please do the following?

  1. Reset your branch to the last master commit and only keep the change in the build Folder.
  2. Run nom run build 18.9.0

After that you should only get the relevant change and then you can force push the branch.

jaxoncreed commented 1 year ago

Thanks for the tip. I reset and ran the command npm run build 18.9.0, but it still resulted in changes to all the files. Do you know if there's something I'm doing wrong?

vweevers commented 1 year ago

It's mostly whitespace changes, I would guess there was some fix in babel or prettier that changes the output here.

jaxoncreed commented 1 year ago

Is this okay to merge then? Is there anything else I should do?

jaxoncreed commented 1 year ago

Hi all, just want to bump this. Can it be merged in?

ShogunPanda commented 1 year ago

Can you please rebase and solve merge conflicts?

jaxoncreed commented 1 year ago

Thanks @ShogunPanda I've rebased and solved merge conflicts

ShogunPanda commented 1 year ago

@mcollina This LGTM. I think you can now have the CI run and then merge.

jaxoncreed commented 1 year ago

Hi all. Just bumping this again to see if it can be merged in.

mcollina commented 1 year ago

I'm a bit swamped atm. Releasing readable-stream is always a potentially dangerous operation (given the amount of downloads) that I want to do with great care.

It's on the list.

jaxoncreed commented 1 year ago

Hey, sorry to pester again. Just want to see if there's a timeline for merging this in.

moki commented 1 year ago

breaks tar-stream@^3 for me

mcollina commented 1 year ago

@moki how? did you have a repro?

jaxoncreed commented 1 year ago

Thank you