webscreens / screen-enumeration

Screen enumeration API explainer
Apache License 2.0
23 stars 4 forks source link

Namespacing under window.screens seems premature and unnecessary #9

Closed domenic closed 4 years ago

domenic commented 4 years ago

This is a pretty minor thing, but I would prefer self.getScreens() over self.navigator.screens.getScreens().

In particular, the motivation provided for the extra level of indirection seems unclear. "This inhibits encapsulation of related screen event APIs later." It's not clear what "encapsulation" is gained here; nothing is hidden/encapsulated. If we hypothesize other APIs related to screens, e.g. self.navigator.screens.somethingElseScreenRelated all you're doing is making people insert a dot between accessing them.

It's also rather redundant. getScreens() already has screens in the name, i.e. is already "namespaced" to be screen-related, so putting it under a screens namespace is not necessary.

Finally, note that the screens object seems to violate the TAG design principles: https://w3ctag.github.io/design-principles/#constructors . Although there is no IDL in this repo, it seems like it's a non-constructible class with a getScreens() method, and not a class that actually represents an instance of something useful that bundles together data and behavior, of which multiple instances could exist and be created.

So in conclusion:

Garbee commented 4 years ago

I think aligning with issue #2 on screen events it seems like we have a tangible understanding that more things relating to this will be needed. Whether it be in v1 or down the road with an updated spec version.

With that in mind, I really like the suggestion of the Web IDL namespace and just saying Screens for access and a simpler method like get for grabbing the data.

My concern however, is it being constructable. I don't see any benefit for developers being able to create multiple objects for this work. Having it in navigator under a namespace gives us a clear singular area to see what is happening within an application with these events. navigator.screens.get() sounds very nice along with navigator.screens.addEventListener('onchange', function() {});.

Is there a benefit to allowing multiple instances to exist each with a separated set of event handlers in the context of this API?

domenic commented 4 years ago

I agree this shouldn't be constructible with multiple instances existing. The implication then, is that it shouldn't be a class. It should be a namespace, or just a method of the global object, instead.

Having it in navigator under a namespace gives us a clear singular area to see what is happening within an application with these events.

I don't see why navigator helps here. And I don't think there's a significant difference between making people type dots versus not, in terms of improving code clarity.

michaelwasserman commented 4 years ago

Thanks for the discussion here! Given the likelihood that developers will want an to be notified on display configuration changes (per issue #2 Screen change events), how would that fit into a Web IDL namespace or self.getScreens() approach? I imagined that it would make more sense to offer a Screens object that is an EventTarget subclass to offer getScreens and addEventListener, etc. together? I'm actually very new to web platform development, and your guidance here is really appreciated!

domenic commented 4 years ago

self.getScreens() + self.addEventListener("screenchange", ...) seems like the simplest way. This is similar to the pattern of navigator.language + self.addEventListener("onlanguagechange", ...) (except more consistent if you put both on self).

michaelwasserman commented 4 years ago

Oh cool, that does seem straightforward. Do you imagine that it's okay to add these to the global namespace in that manner? It seems like comparable device-related apis have found there way onto navigator or other surfaces: USB.getDevices(), Bluetooth.requestDevice(), navigator.getVRDisplays(). I wouldn't want to suggest dumping this into the global namespace if that's inappropriate.

domenic commented 4 years ago

It does seem like a lot of specs are ignoring the TAG advice these days, which kind of makes this into a tradeoff of consistency (with those specs) vs. goodness (of avoiding unnecessary namespacing and non-constructible class instances). I guess I'm now less sure which path is right, although I still lean toward goodness slightly. Maybe we should try to get more opinions before making any changes.

michaelwasserman commented 4 years ago

I'm inclined to pursue your proposed API shape and list other alternative ideas for completeness; thanks for the advice! Do you foresee any arguments against this approach that I could explore? Any problems around adding "screenchange" to the global addEventListener, since that also adds a global property/attribute |onscreenchange|? Please excuse my inexperience with the nuances and tradeoffs here.

domenic commented 4 years ago

Cool, happy to help! I can't think of any strong arguments against this approach at the current time. We've explored ones ranging from potentially-interesting (consistency) to IMO-weak (aesthetic preference for screens. instead of screens as a prefix). I'm happy to get involved and help reassess if some do arise.

michaelwasserman commented 4 years ago

I adopted the suggested API shape as the explainer's leading proposal in a recent change, which should satisfy the main request of this issue. Please reopen this or file another issue if you have further feedback, thanks!