jQuery should be removed as package dependency. #649

Open
benwiley4000 opened this Issue Jun 22, 2016 · 13 comments

Projects

None yet

2 participants

@benwiley4000
Contributor

I'm building an app that uses MetricsGraphics without jQuery. Since jQuery is not required in MetricsGraphics except for tooltips, my build should not fail if I deleted the jQuery package after installing metrics-graphics from npm. A runtime error should be triggered if one attempts to use MetricsGraphics tooltips without jQuery available; otherwise, everything should run smoothly without.

Thoughts? I'd submit a PR, but I'm not 100% familiar with this particular build process.

@almossawi
Member

Correct. You should, additionally, set show_tooltips to false.

@benwiley4000
Contributor

If I set show_tooltips to false then everything won't break? Interesting.. though will this avoid including jQuery in my compiled bundle?

@almossawi
Member

Correct. Here's the relevant block.

@benwiley4000
Contributor

Not sure I follow. Sure that's the only invocation of jQuery, but doesn't this line https://github.com/mozilla/metrics-graphics/blob/master/gulp/index.js#L85 mean jQuery will be required for packaging in the bundle regardless?

@almossawi
Member

Ah, I misread your question. Yes, that would be a more sensible way of doing it. Self-assigning, though feel free to submit a PR in the interim if you like.

@almossawi almossawi self-assigned this Jun 23, 2016
@almossawi almossawi added this to the v2.10 milestone Jun 23, 2016
@benwiley4000
Contributor

I'll leave you to it - don't want to break anything. Thanks! :)
On Jun 23, 2016 5:21 PM, "Ali Almossawi" notifications@github.com wrote:

Ah, I misread your question. Yes, that would be a more sensible way of
doing it. Self-assigning, though feel free to submit a PR in the interim if
you like.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#649 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AM7h7W2J2NhkPxfvPdEI45qAMLo3usEqks5qOvjZgaJpZM4I8L-5
.

@benwiley4000
Contributor

I've been thinking about how this could be done and it seems like the best solution would be to set some sort of global or environment config that would determine if jQuery should be required by MG during module init. Otherwise the user would have to have specified jQuery already as a global.

@benwiley4000
Contributor

Other options: give MG a function for specifying your copy of jQuery. Or: use a non-jQuery, lighter tooltip?

@almossawi almossawi added a commit that referenced this issue Jul 8, 2016
@almossawi almossawi Issue #649 a62495b
@almossawi
Member

That seems reasonable--the referenced commit triggers an error for each chart that uses tooltips when jQuery isn't included. How does that work for you?

@benwiley4000
Contributor
benwiley4000 commented Jul 9, 2016 edited

Unfortunately it doesn't quite solve the problem. jQuery has been removed from the package.json but not from the gulpfile, so if I delete the jQuery module from my repo, I get this error when I bundle:

ERROR in ./~/metrics-graphics/dist/metricsgraphics.js
Module not found: Error: Cannot resolve module 'jquery' in /[parent directories]/node_modules/metrics-graphics/dist
 @ ./~/metrics-graphics/dist/metricsgraphics.js 3:4-37

Additionally - it doesn't look like the jQuery variable is actually defined when jQuery is provided as a module rather than a global dependency. When I tried enabling tooltips and running my app I got the error:

bundle.js:64335 ERROR : ... :  In order to enable tooltips, please make sure you include jQuery.

Although when I changed the metrics graphics code to check for $ rather than jQuery I got a new error:

bundle.js:57568 Uncaught TypeError: $chartTitle.popover is not a function

Hope those error logs help.

Ben

P.S. It might make sense for jQuery to be included in the package.json as a peerDependency (i.e. it's expected to be provided by the project that depends on metricsgraphcis but for npm3+ it's not automatically installed unless the parent project has it as a dependency). This doesn't change anything except gives people the hint to have their project depend on jQuery.

@benwiley4000
Contributor

@almossawi does my explanation of why that fix isn't sufficient make sense?

@almossawi almossawi added a commit that referenced this issue Jul 25, 2016
@almossawi almossawi #649 and #585 ea7a178
@almossawi
Member

Thank you, and apologies for the late reply.

So long as jQuery is being loaded before MG, and unless $ is being redefined, I can't quite think of what the culprit might be. Would you mind seeing if this fix applies?

You may have to change references to jQuery to $; we can switch those references later if necessary and if the above solves your problem.

@almossawi almossawi added a commit that referenced this issue Jul 26, 2016
@almossawi almossawi #649 cleanup 55cdc0c
@almossawi almossawi added a commit that referenced this issue Jul 29, 2016
@almossawi almossawi #649 added jquery_exists c94097e
@almossawi
Member

With the release planned for next Thursday, what are your thoughts on how best to proceed? Were you able to resolve your issue? From what I can tell, the change, as it stands, will be transparent to anyone upgrading. If that's not the case, or if I'm missing something crucial, feel free to point that out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment