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

stream: use an assertion for weird error cases #27073

Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Apr 3, 2019

These two cases should only happen in case there's a bug in our
implementation or the user manipulated things pretty bad. So instead
of using our regular error system for these two cases, use a simple
assertion.

I guess this should be semver-major even though it has always been just a sanity check (from the original implementation on).

@nodejs/streams PTAL

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

These two cases should only happen in case there's a bug in our
implementation or the user manipulated things pretty bad. So instead
of using our regular error system for these two cases, use a simple
assertion.
@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 3, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. stream Issues and PRs related to the stream subsystem. labels Apr 3, 2019
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.

I don’t think we should land this, I would need to add assert to readable-stream dependencies

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 3, 2019

@mcollina I see. In that case you should probably be fine to accept this PR as is if it lands after #26635? It removes the dependency to assert in internal/assert.

@mcollina
Copy link
Member

mcollina commented Apr 4, 2019

I'll keep being -1. I prefer not having to load anything from internal/assert, as then it would be manual, polyfill work for readable-stream.

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 4, 2019

@mcollina in what way would you have to polyfill that part? It only loads an internal error and AFAIC those are now also ported to readable-stream, right?

Do you suggest to keep it as is instead? We could also remove the code completely instead of keeping a sanity check shrug.

@mcollina
Copy link
Member

mcollina commented Apr 4, 2019

@mcollina in what way would you have to polyfill that part? It only loads an internal error and AFAIC those are now also ported to readable-stream, right?

Essentially I would have to write some regular expressions to replace the usage of that module with something that does not need a require.

Do you suggest to keep it as is instead? We could also remove the code completely instead of keeping a sanity check shrug.

Yes, I would prefer to keep this as-is.

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 4, 2019

I suggest that this is looked at again at some point since this is pretty much dead code and not coverable when not done as assertion as I did.

@BridgeAR BridgeAR closed this Apr 4, 2019
@BridgeAR BridgeAR deleted the use-assertion-instead-of-errors branch January 20, 2020 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants