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

refactor: migrate SendStream to ES6 classes #69

Closed
wants to merge 1 commit into from

Conversation

mon-jai
Copy link

@mon-jai mon-jai commented Apr 6, 2024

Checklist

  • run npm run test and npm run benchmark (There is no benchmark script in this package)
  • tests and/or benchmarks are included
  • documentation is changed or added (There are no changes affecting the API)
  • commit message and code follows the Developer's Certification of Origin
    and the Code of conduct

This pull request migrates SendStream to ES6 classes.

Note

  • I have done my best to maintain the original code formatting.
  • All tests and lint checks pass successfully after implementing these changes.

Changes Made

  • Migrated SendStream to ES6 classes.
  • Replaced || with ?? (nullish coalescing operator) wherever appropriate.
  • Removed || false which was redundant ((etag && ifRange.indexOf(etag) !== -1) || false ➡️ (etag && ifRange.indexOf(etag) !== -1)).

Changes to Consider

  • Utilize ES2022 private properties for private methods and properties (_acceptRanges ➡️ #acceptRanges, hasTrailingSlash() ➡️ #hasTrailingSlash()).

@mon-jai
Copy link
Author

mon-jai commented Apr 6, 2024

The change is minimal when whitespace changes are hidden.

image

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

It looks fine but some maintainers do not like the idea of refactoring to classes for no reason, which is understandable as it may introduce subtle bugs

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'm -1 for that reason. You can also see a Proxy creeping in to be feature compatible.

@gurgunday
Copy link
Member

Closing as it's no longer feasible to extend Stream and do such a refactor

But thanks for the effort

@gurgunday gurgunday closed this Jul 12, 2024
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.

3 participants