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

feat: allows to propagete providers named reset to its dependents #196

Merged

Conversation

esroyo
Copy link
Contributor

@esroyo esroyo commented Sep 22, 2023

This PR implements the ideas proposed on issue #193

@young-steveo I think the most "notable" change, is that It is no longer possible to configure the final instance as a direct value in the property descriptor. Regardless of the existence of middleware, the getter is needed to keep track of the accesses to the container.

Although that is an "internal detail", that fact could be relevant enough to represent a major version bump.

Changes to the README are fairly minimal; awaiting for your feedback before investing more work. Thanks!

Copy link
Owner

@young-steveo young-steveo left a comment

Choose a reason for hiding this comment

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

This looks good! I added a couple of comments. Also, could you elaborate on

It is no longer possible to configure the final instance as a direct value in the property descriptor

I'm not sure I follow.

test/spec/provider.spec.js Show resolved Hide resolved
src/Bottle/middleware.js Show resolved Hide resolved
@esroyo
Copy link
Contributor Author

esroyo commented Nov 22, 2023

It is no longer possible to configure the final instance as a direct value in the property descriptor

I'm not sure I follow.

I was alluding to the elimination of the possibility to configure value in the property descriptor here: https://github.com/young-steveo/bottlejs/pull/196/files#diff-fdb247437fd68a61581f78513f2d7b710cfc757b6288e57cfee6e9182d8678fcL30
We would need to go always via the getter function, which in the end is an extra function call for cases when there is no middleware.

@young-steveo
Copy link
Owner

I'm going to call it good. I'm unsure if this requires a major version bump, but I'll investigate before figuring out which version to deploy.

Thanks for all your work!

@young-steveo young-steveo merged commit c9c311b into young-steveo:master Dec 21, 2023
@esroyo
Copy link
Contributor Author

esroyo commented Feb 8, 2024

I'm going to call it good. I'm unsure if this requires a major version bump, but I'll investigate before figuring out which version to deploy.

Thanks for all your work!

My two cents about the version bump:

  • technically this is a clean API addition and would fit in a minor bump.
  • changing a function from one param to two in JS is always delicate. There is a risk that someone has been passing around the function in callback style to Array prototype methods, which usually work nice with one param functions but can introduce mysterious behaviours when using more than one param (typical Array#map(parseInt) problem). Probably not the case for resetProviders, but you never know.
  • Given the chances of this package being used on relatively old or stable code bases, I would opt for a major bump to ensure no risk of unintentional upgrades on those code bases.

I'm grateful to have the opportunity to contribute to this little piece of software, which I have used so much. Thanks to you @young-steveo for the original creation and for keeping this package alive during all this years.

Best regards.

@esroyo esroyo deleted the feat/reset-providers-propagation branch February 14, 2024 17:05
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.

2 participants