Tech registry #2782
Tech registry #2782
Conversation
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: b9ec25231bac4c8cf0c9da438fd7b19d90511b57 (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: bb329136bdc3ab58d7b6a1d811a4c2d2502aa1c7 (Please note that this is a fully automated comment.) |
gkatsev
force-pushed
the
tech-registry
branch
from
bb32913
to
f398471
Compare
gkatsev
force-pushed
the
tech-registry
branch
from
f398471
to
28e20f7
Compare
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 28e20f7 (Please note that this is a fully automated comment.) |
| return Tech.techs_[name]; | ||
| } | ||
|
|
||
| if (window && window.videojs && window.videojs[name]) { |
There was a problem hiding this comment.
I feel like there should probably be a Tech.isTech(window.videojs[name]) condition here to prevent misleading log.warn calls.
There was a problem hiding this comment.
What should it do if it isn't a tech? Component doesn't do that checking currently either and I basically just copied the code from there.
I think it makes sense to warn/throw if you're trying to register something as a tech that isn't, this function could be slightly more permissive.
There was a problem hiding this comment.
Not 100% sure on that - or that anything needs to be done, given more reflection.
My original thinking was that if someone were to reach this with a value that wasn't a tech, but was available on the window, it would suggest they were doing something really weird and we shouldn't say "don't access your techs like this". Perhaps the log.warn is enough to suggest they did something strange/wrong.
There was a problem hiding this comment.
Decided not to do anything here but throw if trying to register a non-tech as a tech.
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: f39847122102900adbd438c1655b22194ccd7223 (Please note that this is a fully automated comment.) |
|
The last pam message is for an older build. |
| * Registers a Tech | ||
| * | ||
| * @param {String} name Name of the Tech to register | ||
| * @param {Object} tech The tecj to register |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 25f7d80 (Please note that this is a fully automated comment.) |
| let tech = Tech.getTech(techName); | ||
|
|
||
| if (!tech) { | ||
| tech = Component.getComponent(techName); |
There was a problem hiding this comment.
This would mean that the tech was only registered as a component? I think it's worth a comment above this to remind ourselves to one day get rid of it.
There was a problem hiding this comment.
Yep, this is basically to support the now-deprecated feature of registering techs as components.
|
One tiny comment, otherwise LGTM |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: e0b9824 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 3d832d9 (Please note that this is a fully automated comment.) |
|
@pam retry |
|
I just realized I need to do the check for Tech.getTech/getComponent in a few other places :/ |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: e0b9824 (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: c58e037 (Please note that this is a fully automated comment.) |
This depends on #2773 but adds in a Tech registry which fixes #2772.