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: rename fn.bind to fn.bind_ to strongly indicate it should not be used externally #7940

Merged
merged 4 commits into from
Oct 21, 2022

Conversation

misteroneill
Copy link
Member

Description

The utils/fn.bind function will be exposed in Video.js 8 because fn is exposed as videojs.fn. We don't want people using it, but it cannot be removed without breaking the Video.js event system.

This PR adds a _ at the end of its name to indicate that it is private.

No external updates should be needed as it was not directly exposed. The previously-supported videojs.bind has been deprecated.

Specific Changes proposed

  • Rename utils/fn.bind to utils/fn.bind_
  • Update usages

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@gkatsev
Copy link
Member

gkatsev commented Sep 29, 2022

Rename to Fn.bind_but_dont_use_or_you_will_be_fired

@codecov
Copy link

codecov bot commented Sep 30, 2022

Codecov Report

Merging #7940 (bc91650) into next (b5fefde) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             next    #7940      +/-   ##
==========================================
+ Coverage   81.96%   81.97%   +0.01%     
==========================================
  Files         111      111              
  Lines        7356     7356              
  Branches     1776     1776              
==========================================
+ Hits         6029     6030       +1     
+ Misses       1327     1326       -1     
Impacted Files Coverage Δ
src/js/component.js 94.11% <100.00%> (ø)
...control-bar/progress-control/mouse-time-display.js 63.63% <100.00%> (ø)
.../control-bar/progress-control/play-progress-bar.js 88.23% <100.00%> (ø)
...s/control-bar/progress-control/progress-control.js 26.66% <100.00%> (ø)
src/js/control-bar/progress-control/seek-bar.js 55.19% <100.00%> (ø)
...rc/js/control-bar/progress-control/time-tooltip.js 84.61% <100.00%> (+2.56%) ⬆️
...rol-bar/text-track-controls/descriptions-button.js 100.00% <100.00%> (ø)
src/js/control-bar/track-button.js 94.11% <100.00%> (ø)
...l-bar/volume-control/mouse-volume-level-display.js 53.84% <100.00%> (ø)
...rc/js/control-bar/volume-control/volume-control.js 48.78% <100.00%> (ø)
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@misteroneill misteroneill merged commit fa57723 into next Oct 21, 2022
@misteroneill misteroneill deleted the private-bind branch October 21, 2022 16:19
misteroneill added a commit that referenced this pull request Nov 23, 2022
misteroneill added a commit that referenced this pull request Nov 23, 2022
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants