Vendor jquery and lodash, introduce EventEmitter3 #1193

vincemtnz · 2018-06-20T08:31:39Z

Note: PR SumOfUs/sou-assets#58 must be merged before this.

This PR reomves the cdn version of jQuery and instead it adds it to a new "globals" pack, which does the following:

Imports jQuery from NPM, and assigns it to window.$ and window.jQuery.
Imports lodash from NPM and assigns it to window._.
Imports EventEmitter3 instance and assigns it to window.event;
Extends window.$ with two methods: publish and subscribe, to maintain some backwards compatibility with $.publish and $.subscribe. These methods use EventEmitter3 internally.
Finally, it loads jquery plugins that didn't work without a global jQuery factory, this time successfully as we now have new rules on how jquery is imported.

There are new rules on how jquery, and lodash are imported: if you're importing the module from within the app/javascript/vendor folder, webpack returns the npm module as you would expect, but when you import jquery/lodash from anywhere else, webpack marks them as externals, i.e. window.$ and window._. This ensures that once we've imported jQuery and added it to the global scope, other modules are modifying the same instance (e.g. jquery-sticky, selectize, etc). You can see how the webpack module is marked as an external conditionally here: https://github.com/SumOfUs/Champaign/pull/1193/files#diff-d725c81d79986543575ead6053b79a54R18

EventEmitter 3

I'm replacing our jQuery-dependent pub/sub functions for EventEmitter 3. It has no external dependencies and is 1kb minified+gzipped. It's also better alternative that is compatible with the Node.js events API.

To register event handlers, use ee.addEventListener or ee.on. To emit an event, use ee.emit. For convenience, $.publish wraps ee.emit and $.subscribe wraps ee.on, so there is some degree of backwards compatibility. Subscribed events using this method can no longer rely on the jQuery event object.

# Before / after comparison of publishing an event
# Note that with EventEmitter3 we don't need to place arguments in an array.
- $.publish('namespace:event', [var1, var2, var3]);
+ window.event.emit('namespace:event', var1, var2, var3);

# Before/after comparison of subscribing to an event. Note that with EventEmitter3
# we don't receive a jQuery event object as the first parameter anymore.
- $.subscribe('namespace:event', function(event, var1, var2, var3) { ... });
+ window.event.on('namespace:event', function(var1, var2, var3) { ... });

Other changes

Adds the // @flow [weak] pragma to most files under app/javascript/member-facing and app/javascript/campaigner-facing (except for those that would've introduced too many errors). These files were previously not checked so there were no guarantees that we wouldn't introduce errors inadvertently when modifying them.
Replaces $.publish and $.subscribe for ee.emit and ee.on in all JavaScript sources, and uses the global window.event.emit/on in all template code. Note: there are still pages in production with custom JavaScript code that rely on $.publish/subscribe, so I've kept both for backwards compatibility, however, they should be ported ASAP. This PR therefore depends on Move from $.subscribe to window.event.on sou-assets#58

PD: Although the PR lists 100 files changed, most of them only change a few lines where $.publish or $.subscribe have been updated to ee.emit or ee.on. The more relevant files worth taking a look at when reviewing this PR are listed here:

Removes the cdn version of jQuery and adds it to the `globals` pack, which loads jQuery, initialises `window.event` (to replace $.publish and $.subscribe in the future), and loads third party jQuery plugins to the global scope. EventEmitter 3 ============== I'm replacing our jQuery-dependent pub/sub functions for EventEmitter 3. It has no external dependencies and is 1kb minified+gzipped. It's also better alternative that is compatible with the Node.js events API. To register event handlers, use `ee.addEventListener` or `ee.on`. To emit an event, use `ee.emit`. Github: https://github.com/primus/eventemitter3 Node.js Events: https://nodejs.org/api/events.html *Note:* For convenience, `$.publish` wraps `ee.emit` and `$.subscribe` wraps `ee.on`.

Tuuleh

Looks good to me - do you want me to run and test this somehow?

Could you just do us a solid and write a short summary on what's changed - e.g. not using jquery events anymore, using event emitter instead, what's in the global packs

Tuuleh · 2018-06-20T14:50:46Z

app/javascript/campaigner_facing/page_edit_bar.js

-import _ from "lodash";
+import $ from 'jquery';
+import I18n from 'champaign-i18n';
+import Backbone from 'backbone';


How come it worked without all these imports?

We had window.$ loaded via CDN - but we're getting a lot of errors on sentry on jQuery not being present, which was the main reason for this change in how we load jQuery.

Tuuleh · 2018-06-20T14:51:47Z

app/javascript/campaigner_facing/shares_editor.js

-  $('.shares-editor__existing').on('click', '.share-copy-url', e => {
-    e.preventDefault();
-  });
+  $('.shares-editor__existing').on(


ack, I will need to rebase with your branch

Tuuleh · 2018-06-20T14:54:40Z

app/javascript/member-facing/backbone/facebook_share_view.js

-      this.model.post(FB, () => {
-        this.trackEvent("shared");
+      this.model.post(window.FB, () => {
+        this.trackEvent('shared');


Are all these single quotations / splitting things into new lines Rubocop changes?

They're prettier which will autoformat on commit.

Tuuleh · 2018-06-20T14:59:23Z

app/javascript/packs/recommend_pages.js

@@ -61,7 +63,7 @@ const CampaignTile = props => (
        })}
      </div>
    </div>
-    <div className="campaign-tile__lead">{props.title}</div>
+    <div className="cggggampaign-tile__lead">{props.title}</div>


Did you mean to rename this class so wonky?

😨 me failing at VIM

Tuuleh · 2018-06-20T15:03:38Z

config/webpack/custom.js

-    jquery: 'window.$',
-    'champaign-i18n': 'window.I18n',
+  externals: [
+    /* Replace any jquery import/require statements for the global `$` object,


Thanks for this very illustrative comment

* Move recommend_pages.js into the member-facing pack and out of its own pack. Reduces size (boilerplate) and number of requests. Also change initialisation to use events (avoid more globals) * Move event_tracking.js into the member-facing pack and out of its own pack.

osahyoun · 2018-06-21T09:43:52Z

💯

rodrei · 2018-06-25T13:29:10Z

app/liquid/views/partials/_recommend_pages.liquid

      locale: '{{locale}}',
      pageId: "{{ id }}"
-    };
-   mountRecommendPages('recommend-pages-component', data);


Does this mean we're no longer mounting components by calling a function? we're now emitting an event?

I think overall this is a cleaner approach that's less error prone, but I've only updated recommend pages for now as the feature is the newest and meant the least effort. Using events we avoid throwing errors in the template that may break the entire page.

I also refactored the RecommendPages component as it didn't need its own pack (so I've added it to member_facing.js) and it's also less error prone.

vincemtnz added 3 commits June 20, 2018 09:33

Alias jquery to window.$, except for app/javascript/vendor/*

c7ae5cf

Also vendor lodash (using NPM, append to global)

b4c0f15

vincemtnz force-pushed the feature.jquery-not-a-func branch from 31c34b3 to b4c0f15 Compare June 20, 2018 08:35

vincemtnz requested review from osahyoun, rodrei and Tuuleh June 20, 2018 10:01

vincemtnz self-assigned this Jun 20, 2018

vincemtnz added enhancement refactor labels Jun 20, 2018

Tuuleh reviewed Jun 20, 2018

View reviewed changes

vincemtnz merged commit f83f9ab into development Jun 20, 2018

vincemtnz deleted the feature.jquery-not-a-func branch June 20, 2018 16:23

rodrei reviewed Jun 25, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vendor jquery and lodash, introduce EventEmitter3 #1193

Vendor jquery and lodash, introduce EventEmitter3 #1193

vincemtnz commented Jun 20, 2018 •

edited

Tuuleh left a comment

Tuuleh Jun 20, 2018

vincemtnz Jun 20, 2018

Tuuleh Jun 20, 2018

Tuuleh Jun 20, 2018

vincemtnz Jun 20, 2018

Tuuleh Jun 20, 2018

vincemtnz Jun 20, 2018

Tuuleh Jun 20, 2018

osahyoun commented Jun 21, 2018

rodrei Jun 25, 2018

vincemtnz Jun 25, 2018

Vendor jquery and lodash, introduce EventEmitter3 #1193

Vendor jquery and lodash, introduce EventEmitter3 #1193

Conversation

vincemtnz commented Jun 20, 2018 • edited

EventEmitter 3

Other changes

Tuuleh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osahyoun commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincemtnz commented Jun 20, 2018 •

edited