Allow bundled geo.js to work in Candela

[waxlamp]

This change is enough to let Candela make use of GeoJS as a dependent library. However, I'm not sure how to test whether this breaks existing use cases (e.g., Minerva). Submitting this as a WIP PR mainly to have a discussion about it.

[codecov-io]

Current coverage is 71.75%

Merging #572 into master will increase coverage by +<.01%

@@             master       #572   diff @@
==========================================
  Files            82         82          
  Lines          7070       7072     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           5072       5074     +2   
  Misses         1998       1998          
  Partials          0          0          

Powered by Codecov. Last updated by 1db67be...ae10539

[jbeezley]

Thanks for looking into this. As I mentioned, I'm okay with making this change in general. To reiterate what I've said before, here are the problems that I see with this:

1. +84KB minified bundle size. I'm willing to accept this especially if we plan to migrate away from using jQuery internally in favor of smaller, more modular libraries or ES6 primitives.
2. The jquery objects returned by geo.jQuery or map.node(), etc will not necessarily be the global jQuery instance, so it won't always have the global plugins installed on them. This is a minimal complication that can be solved easily (i.e. $(map.node().get(0)).myPlugin()), but I can see it as a source of confusion.
3. If several libraries included on a page start doing things like this, the situation can become unmanageable... The particular jquery object in use at any given time can depend on the order in which libraries are loaded.
4. Some frameworks (meteor as an example) attempt to isolate individual components by messing with global objects so there become subtle difference between $, window.$, and this.$ at the global scope. I think this is one reason why most UMD patterns usually set globals on this rather than window. I'm not sure of the implications of doing so here though.

The only other comment I have is that we should probably remove jquery from the vendor bundle and add a copyright statement in the main bundle.

[aashish24]

@ronichoudhury thanks for this R. I am fine with this change as discussed as long as @jbeezley is happy with it. We will eventually move to es6 but will have to think carefully on removing complete dependency on JQuery.

[waxlamp]

On Sat, May 7, 2016 at 1:09 PM Jonathan Beezley [email protected] wrote:

Thanks for looking into this. As I mentioned, I'm okay with making this
change in general. To reiterate what I've said before, here are the
problems that I see with this:

1. +84KB minified bundle size. I'm willing to accept this especially if we
   plan to migrate away from using jQuery internally in favor of smaller, more
   modular libraries or ES6 primitives.

I also think code-splitting in Webpack can help with this (though I don't have much direct experience with it yet in Candela). As for ES6 and replacing jQuery usage, I can help with that if needed. In particular, I've figured out the delicate balance of configuration needed to do unit, integration, and coverage testing with a Webpack/ES6 codebase.

1. The jquery objects returned by geo.jQuery or map.node() , etc will not
   necessarily be the global jQuery instance, so it won't always have the
   global plugins installed on them. This is a minimal complication that can
   be solved easily (ie $(map.node().get(0)).myPlugin() ), but I can see it as
   a source of confusion.

Maybe careful documentation could help with this. Also, if it's a niche use-case that leads to the confusion (namely, what I'm doing with Candela), then some kind of FAQ entry could easily cover the explanation of what's happening.

1. If several libraries included on a page start doing things like this, the
   situation can become unmanageable... The particular jquery object in use at
   any given time can depend on the order in which libraries are loaded.

I'm still having trouble understanding when this might arise in this way. If I'm using jQuery in my page without knowing that, eg, GeoJS is using it internally, I'm not sure why an interaction between the two jQueries would arise in the first place. This is not to say I think it's impossible, I just have trouble picturing when it might happen.

1. Some frameworks (meteor as an example) attempt to isolate individual
   components by messing with global objects so there become subtle difference
   between $ , window.$ , and this.$ at the global scope. I think this is one
   reason why most UMD patterns usually set globals on this rather than
   window . I'm not sure of the implications of doing so here though.

this resolves to window in web environments, and something else in non-web environments, right? I'm not sure if the use of jQuery implies a web environment or not, but we could change the code to be slightly safer with something like if (window && !window.jQuery) { ... } until we determine whether it should just attach to this instead.

The only other comment I have is that we should probably remove jquery from
the vendor bundle and add a copyright statement in the main bundle.

I will take care of this forthwith.

[waxlamp]

@jbeezley, @aashish24, what more should I do before I can remove the WIP from this and get it ready for merge? Is there a suitable test that could be added? As I mentioned in my original comment, I'm not totally sure of the path to testing existing use cases. Let me know what you think.

[jbeezley]

I also think code-splitting in Webpack can help with this...

Webpack's code splitting is meant for use cases where you have a multipage web app. You can split out common things like vendor bundles that is shared between pages, and thus cached by the browser. It won't help in this case because that vendor bundle has to always be included with the application bundle, it can't fall back to anybody else's vendor bundle or to globals. For this purpose, code splitting would only result in requiring users of the library to include two files rather than one.

I'm still having trouble understanding when this might arise...

Any example I can think of seems kind of contrived. My unease is due to my experiences dealing with how different libraries and module bundles handle jquery specifically. Sometimes they try to be smart and attach to the global jquery object, sometimes they overwrite it, sometimes they try and merge them... My point isn't that what is done here is wrong, but that what is done elsewhere causes endless complications when there is more than one jquery on the page. It is a problem specifically for jquery because it is so ubiquitous and its design (specifically how you extend it via $.fn.) is inherently incompatible with modules.

this resolves to window in web environments, and something else in non-web environments, right?

That's not what I'm getting at. Take this for example, which is a rough approximation of what Meteor does (or used to do). Other bundlers may very well do something similar.

// clone `window` somehow (I don't think this will actually work)
var copy = $.extend({}, window);

function block() {
  // <-- your code bundle is inserted here
}

block.call(copy); 

In this case, window and this are different, which causes headaches when the module loader does things like this.

As I mentioned though, I'm okay with this as is. I just want to make sure that it doesn't break existing use cases before merging.

[waxlamp]

I more or less understand the concerns about the stuff you responded to. Basically, we'll just need to keep this in mind going forward and check whether it's involved with any weird behaviors we see in the future.

I'm going to go ahead and remove the WIP label from this and wait for you to confirm that existing use cases will be ok.

Thanks for your support on this!

[jbeezley]

This works with Minerva, I'll go ahead and give it a LGTM. Thanks!

[jbeezley]

FYI The selenium tests have started to fail this morning probably due to a firefox update. There aren't any problems with this branch. I'll look into it later today.

[waxlamp]

Re moving away from jQuery for things like extend() - I just learned about this from Tristan: https://github.com/tjmehta/101. I haven't tried it myself but it's designed to have minimal overlap with vanilla JS, and is thoroughly modularized (each method lies in its own module). Don't know if you guys have seen it but it looks interesting in any case.