Stop modifying String.prototype, _.prototype, and google.maps.prototype

[wylieconlon]

Apologies if this is a duplicate, I searched through a fair number of open/closed issues and pull requests to find a similar issue.

I think modifying the prototype of objects that you don't own is a fundamentally wrong way to design a library. For example, what if I were to define my own String.prototype.contains method? Would your library stop working? Would my application break? It can't go both ways.

From what I can see of the history of this project (7b6adde), it seems like there was a recent change in prefix from ngGmap to uiGmap. I can see why a .ns() function on the string prototype was useful for that, but I'm sure that a find and replace would have taken the same effort.

In a more angular-specific case, I think this makes your library much harder to understand at first. I was looking at your documentation and I saw this line:

//'GoogleMapApiProvider'.ns() == 'uiGmapGoogleMapApiProvider'
.config(['GoogleMapApiProvider'.ns(), function (GoogleMapApi) {

Since there are multiple ways to annotate dependencies, and dependency annotation is optional, I think this is unnecessarily confusing. Instead, I would propose that the documentation should not use custom prototype functions, and look like this instead (matching the Angular documentation):

.config(function (uiGmapGoogleMapApiProvider) {

The same argument applies to your custom extension of lodash and google maps. Why not wrap their code with your library-specific code? Other applications don't need to be aware of those extensions.

[nmccready]

With lodash and with contains I agree. And for the most part I think the lodash extensions can maybe go away. However with regards to .ns() I disagree, the amount of time it took for me to originally implement it it was nil, and the amount of time it saved me was huge. I made it cause I planned on a rename being possible (which could have happened a third time). From the standpoint of examples yes they should possibly be more terse. However I have to maintain them. Constantly fixing broken examples due issues like that is a huge pain in the rear.

As a alternative ns could be.

ns = (str, toPrepend = "uiGmap") ->
    toPrepend + str

I really do not want to make the services more terse and have more maintenance. So I can meet you possibly 75% of the way. However, is this urgent if so please make a PR.

[nmccready]

Is there a way to modify prototypes internally to not affect the global space? I am thinking more along the lines of requirejs / commonjs (via webpack or something).

[cthrax]

You could make namespace a function that takes a string, rather than modifying string itself. And pass that function around.

[nmccready]

@cthrax right that is what the change of ns('Marker') will yield uiGmapMarker

[cthrax]

👍

[nmccready]

Sorry I love prototype chaining it looks so much cleaner :P ,

We could have a Function that returns an object and behaves similarly to string prototypes.

OurString = (str) ->
   _str = str
   @ns = (prepend + 'uiGmap') =>
      prepend + _str
      @
   @contains= (junk) =>
       _str.indexOf(value,fromIndex) != -1
   @value = ->
      _str
    @

someStr = new OurString('junk').ns('crap').ns('uiGmap').contains('crap') # yields true
#internal _str, "uiGmapcrapjunk"

[wylieconlon]

I don't think there is a way to modify the String prototype for just your app, but using something like requirejs or commonjs lets you use closures. You could also use an IIFE, so defining an ns() function wouldn't necessarily get exposed to the global scope. Or, like your example, you could use a new constructor function.

If I were doing this, I would wrap the entire library in an IIFE to limit the helpers, like ns(), to your local scope. What approach do you think works best with this project?

[nmccready]

IIEF like above is my pref

[aesnyder]

I have to admit when I saw the modification of the string prototype I immediately began searching for an alternative library

I personally would just rename all the directives to have uiGmap in front of them. If that's not acceptable my second choice would be:

uiGmap = (str) -> "uiGmap#{str}"

But imo, even that is a pollution of the global name space

As for the lodash and google maps concerns, I'd need to do a bit more digging to formulate an opinion.

[nmccready]

hmm.. well a crazy approach would be grunt.. w/ replace. OR F it. Put it in.

use grunt replace

angular.module("{namespace}google-maps").directive "{namespace}Circle", 

grunt replace {namespace} with uiGmap.. fin

[nmccready]

BTW the google maps prototype mods came from code that was in googlemaps tools. Also the isOpen was added because of suggestion on stack overflow. The entire MarkerLabels and many of those tools all do similar things as well.

[cthrax]

The only concern I would have about grunt-replace is that it makes it difficult to include the modules in client code. Maybe only namespace the directives and not the module names?

[nmccready]

Right but would the modules that people use as the end result be javascript? So it would be a mute point?

[nmccready]

Unless they are using the coffee.