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

fix: removing bind and refactoring consumer #478

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

mogu4iy
Copy link
Contributor

@mogu4iy mogu4iy commented Mar 18, 2024

Resolves #NUMBER

Description:

There is an inheritance issue, that it's not possible to extend Consumer class. It happens because you autoBind all propertyNames from constructor.prototype, so while extending these properties are not accessible anymore in this way.

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Why is this change required?:

There are cases when we what to extend Consumer class, to pass pre defined options for example.
I have created minimal reproducible exaple.

Code changes:

  • Add missing type declarations
  • Remove autoBind usage to let extend this class
  • Update passing in-class methods to intervals using arrow functions to solve context passing issue

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

- Add missing type declarations
- Remove autoBind usage
- Update passing in-class methods to intervals using arrow functions
@mogu4iy mogu4iy requested review from a team as code owners March 18, 2024 16:52
@mogu4iy
Copy link
Contributor Author

mogu4iy commented Mar 18, 2024

@nicholasgriffintn I hope you can check this as well ;)

@nicholasgriffintn
Copy link
Member

Hey, thanks for the PR.

I'm not too sure the reasoning behind the bind, and if we need it or not honestly, it's before my time.

I'll have to have a look through this when I have a spare hour or two, bare with :).

@mogu4iy
Copy link
Contributor Author

mogu4iy commented Mar 18, 2024

Hey, thanks for the PR.

I'm not too sure the reasoning behind the bind, and if we need it or not honestly, it's before my time.

I'll have to have a look through this when I have a spare hour or two, bare with :).

It was used to avoid passing class methods to intervals or promise then calls using arrow functions.

@nicholasgriffintn
Copy link
Member

Yeh it was part of the original commit years ago, so potentially just old Node/JS thoughts. I'll have a proper look through tomorrow.

Copy link

codeclimate bot commented Mar 18, 2024

Code Climate has analyzed commit 3a292f2 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 98.3% (0.0% change).

View more on Code Climate.

@nicholasgriffintn nicholasgriffintn changed the title Fix refactoring consumer fix: removing bind and refactoring consumer Mar 18, 2024
@mogu4iy
Copy link
Contributor Author

mogu4iy commented Mar 21, 2024

@nicholasgriffintn any updates?

@mogu4iy
Copy link
Contributor Author

mogu4iy commented Mar 21, 2024

I am also interested in estimates, when you are going to release 10.0.0

Copy link
Member

@nicholasgriffintn nicholasgriffintn left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, got stuck with the day job. This will be in v10.0.0-canary.3 and eventually v10.

We don't have a date for version 10 yet, ultimately it will be whenever we feel confident to release it without risk of issues, we've not yet done the testing for that, in the meantime, the canary version can be installed.

@nicholasgriffintn nicholasgriffintn merged commit e7ef075 into bbc:main Mar 21, 2024
7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants