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

Are Techs Components? #2772

Closed
gkatsev opened this issue Nov 3, 2015 · 6 comments
Closed

Are Techs Components? #2772

gkatsev opened this issue Nov 3, 2015 · 6 comments
Labels
needs: discussion Needs a broader discussion question

Comments

@gkatsev
Copy link
Member

gkatsev commented Nov 3, 2015

While working on trying to make a generic solution for #2767 I ran into a problem creating components from a list of names. Namely, there isn't a good way to know whether a component is a component or a tech. There is also (at least) one major difference between the two -- a component takes the player and options as arguments but techs take options and ready as arguments. Meaning that you can initialize a tech and a regular component dynamically the same way.
So, the question is whether we should be registering Techs as components or do we need a tech registry?

let componentName = 'Button';
let C = Component.getComponent(componentName);
let c = new C(player, options);

componentName = 'Html5';
C = Component.getComponent(componentName);
c = new C(player, options);  // error in some cases since Html5 assumes that the player is the options
@gkatsev gkatsev added needs: discussion Needs a broader discussion question labels Nov 3, 2015
@dmlap
Copy link
Member

dmlap commented Nov 3, 2015

My feeling: based on the re-arch of techs we did in 5.0, it doesn't make sense for Techs to be Components. They have a lot in common but we did intentionally choose not to pass Techs references to the player.

@misteroneill
Copy link
Member

I'm with @dmlap - things that are the same should be the same, things that are different should be obviously different.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 3, 2015

I agree. Unfortunately, we can't remove Html5 and Flash from the component registration currently because semver.
Should we add a registerTech/getTech to mirror components in a future minor update?

@misteroneill
Copy link
Member

I think that makes sense.

I guess that means until 6.0, registerTech/getTech would also register the tech as a component and getComponent would call getTech if no component is found.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 3, 2015

we only need to maintain the compat for the built-in techs, Html5 and Flash. So, we could just register both as Techs and Components but have others only register Techs as Techs and not Components.

This was referenced Nov 3, 2015
@gkatsev
Copy link
Member Author

gkatsev commented Nov 5, 2015

I have a PR for this: #2782

@gkatsev gkatsev closed this as completed in b1e8636 Nov 9, 2015
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs: discussion Needs a broader discussion question
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants