search

googlearchive / TemplateBinding

TemplateBinding Prolyfill
290 stars 61 forks source link

Do not create excess globals; consider collecting APIs under a namespace as needed #160

Closed sjmiles closed 10 years ago

sjmiles commented 10 years ago

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.

rafaelw commented 10 years ago

Presumably you're currently looking at offending global names. Can you list them?

sjmiles commented 10 years ago

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.

jmesserly commented 10 years ago

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:

https://github.com/Polymer/observe-js/blob/9ac422a456bcbaf5cf61b966d29c55088ff57e9e/src/observe.js#L1589

(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... ;-) )

sjmiles commented 10 years ago

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.

jmesserly commented 10 years ago

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 :) )

jmesserly commented 10 years ago

moved to https://github.com/Polymer/observe-js/issues/66