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

5144 rename throttle component changed #5151

Merged

Conversation

diarmidmackenzie
Copy link
Contributor

Description:

PR as per #5144

Changes proposed:

  • name throttleComponentChanged -> throttleLeadingAndTrailing
  • leave throttleComponentChanged utility function for 1.3.0 back-compatibility
  • add documentation for throttleLeadingAndTrailing in utils API docs.

Copy link
Contributor

@vincentfretin vincentfretin 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 to me.

* Identical to throttleLeadingAndTrailing.
* Exists for back-compatibility with 1.3.0.
*/
module.exports.throttleComponentChanged = module.exports.throttleLeadingAndTrailing;
Copy link
Member

@dmarcos dmarcos Nov 15, 2022

Choose a reason for hiding this comment

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

I think we can just rename. Don't think many were relying on this. We can add a deprecation in next release notes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK - removed this. What needs to happen to make sure this gets tracked for the release note?


A context such as `this` can be provided to handle function binding for convenience.

The same as [lodash's`throttle`][lodash], with `leading` and `trailing` options both set to `true`
Copy link
Member

Choose a reason for hiding this comment

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

wonder if this reference is a liability long term in case lodash API changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but the existing docs already reference loadash throttle (the same link)

See: https://aframe.io/docs/1.3.0/core/utils.html#aframe-utils-throttle-function-minimuminterval-optionalcontext

This doesn't make the situation any worse, I think.

@dmarcos
Copy link
Member

dmarcos commented Nov 18, 2022

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.

3 participants