-
Notifications
You must be signed in to change notification settings - Fork 54
Do not create excess globals; consider collecting APIs under a namespace as needed #160
Comments
Presumably you're currently looking at offending global names. Can you list them? |
I'm not currently looking, that's the bit about the missing due diligence. But this is one of those things that was rattling around in my bean for a long time so I just wanted to get something down into issues. |
fyi, I don't think TemplateBinding creates globals (it just patches HTMLTemplateElement and things like that) ... however, I did noticed recently that observe-js has quite a few globals: (I was actually happy to discover this, since it makes it possible for me to prototype JS+Dart data binding interop without needing any patches to platform.js ... for example I could patch PathObserver and I think TemplateBinding would pick up the patched version. I didn't actually try yet though, and not sure if ultimately that should be considered a feature... ;-) ) |
Ok, my bad if I put this issue in the wrong place. Let's be clear that surfacing an API is not the same as creating a global. I'm not saying we need to privatize more API, just that we should collect these things into namespace objects. |
no worries :). agreed that namespacing is good! (minor note: it does tend to make it impossible to patch the constructor itself, for example, if this is TemplateBinding.js: (function(scope) {
var Path = scope.observe.Path;
var CompoundObserver = scope.observe.CompoundObserver;
...
})(...); ...since observe-js is compiled with TemplateBinding into the same platform.js, it's not possible to swap out the "CompoundObserver" constructor anymore. Whereas if it's always looked up from "window" I could do that. But I yeah, it's not worth messing up the API design for that reason :). And always qualifying it like observe.Path would be ugly. Anyway, forget I brought this up :) ) |
moved to googlearchive/observe-js#66 |
I don't actually know for sure which libraries are doing this, but I wanted to capture the issue without doing the due diligence (sorry!).
In general, we like to be very circumscribed about the global namespace and create as few globals per new API as possible.
The text was updated successfully, but these errors were encountered: