Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed numeric separator #491

Merged
merged 2 commits into from Jan 1, 2023
Merged

Removed numeric separator #491

merged 2 commits into from Jan 1, 2023

Conversation

jaxoncreed
Copy link
Contributor

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. facebook/metro#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
Copy link
Member

mcollina commented Nov 3, 2022

Could you make this change inside the build/ scripts?

@jaxoncreed
Copy link
Contributor Author

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?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

mcollina commented Nov 4, 2022

@ShogunPanda PTAL

@ShogunPanda
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@jaxoncreed
Copy link
Contributor Author

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

@jaxoncreed
Copy link
Contributor Author

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

@ShogunPanda
Copy link
Contributor

Can you please rebase and solve merge conflicts?

@jaxoncreed
Copy link
Contributor Author

Thanks @ShogunPanda I've rebased and solved merge conflicts

@ShogunPanda
Copy link
Contributor

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

@jaxoncreed
Copy link
Contributor Author

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

@mcollina
Copy link
Member

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
Copy link
Contributor Author

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

@moki
Copy link

moki commented Dec 29, 2022

breaks tar-stream@^3 for me

@mcollina
Copy link
Member

mcollina commented Jan 1, 2023

@moki how? did you have a repro?

@mcollina mcollina merged commit f135e4b into nodejs:main Jan 1, 2023
@jaxoncreed
Copy link
Contributor Author

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants