New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to lodash 4 #9684

Merged
merged 9 commits into from Jul 25, 2016

Conversation

Projects
None yet
5 participants
@islemaster
Member

islemaster commented Jul 23, 2016

Upgrades lodash from 3.10.0 to 4.13.1.

I carefully worked through the 4.0.0 upgrade guide, though by itself that wasn't sufficient to fix all of the apps tests; there were a few later changes that broke things too.

Affected by the upgrade

  • The optional deep argument to _.clone() has been removed; we now use _.cloneDeep() instead of _.clone(_, true).
  • The one-argument behavoir of _.zipObject() (accepting data in the format [['key', 'value'], ['key', 'value']]) has been moved to a new method _.fromPairs.
  • _.debounce went through several changes; see below.
  • The _.contains alias for _.includes has been removed.
  • Certain lodash methods that accepted a callback used to follow the JavaScript convention of also accepting a context argument (e.g. the second arugment in _.forEach(function () {...}, this)). This (already optional) context argument has been removed across the board - consumers are expected to bind their callbacks by some other means (like Function.prototype.bind or arrow functions).

What's up with _.debounce?

A few things: The official breaking change in the 4.0 upgrade is that _.debounce no longer accepts a boolean as its third argument. Way back in Lo-Dash (sic) 1.0.0 debounce's third argument was just a boolean immediate that invoked the wrapped function on the leading edge of the timeout instead of the trailing edge. Equivalent behavior now passes an options object {leading: true, trailing: false}. This was an easy fix.

However, _.debounce has been touched a ton since v4.0.0, sometimes in breaking ways:

  • 4.3.0 says "Improved accuracy of _.debounce and _.throttle."
  • 4.4.0 says "Ensured a debounced maxWait timeout isn’t processed on a leading call if leading is false & there isn’t a max delay queued".
  • 4.7.0 says "Refactored _.debounce to simplify, reduce timers, & fix bugs"
  • Worst of all, 4.11.1 says "Ensured _.debounce doesn’t immediately call func when wait is 0." This a breaking change for us, it broke our tests where I often pass zero to bypass request coalescing and throttling in NetSim unit tests. Before, calling _.debounce(func, 0)() would immediately execute func; now it behaves like setTimeout(func, 0) and doesn't invoke until the next tick. I updated my tests to expect this (basically, doing setTimeout(func, 0) before verifying the effects of the debounced method) and we are back up and running. In practice, outside of tests this change shouldn't have any effect.

islemaster added some commits Jul 22, 2016

Fix cases of _.clone(_, true)
The [`deep` argument of `_.clone` has been removed](https://lodash.com/docs#clone); use `_.cloneDeep` in these cases.
Update uses of _.debounce
The boolean `immediate` option for `_.debounce` has been deprecated, so we should use the `leading` and `trailing` options explicitly.
Fix netsim locale import in karma:entry tests
Following the same pattern we did for global locale loading.  That was working for netsim tests when you ran the whole suite, but if you used the --entry option some tests would never try to load the global locales, only netsim locales, and it would break again.
Fix netsim timing tests [test ui] [test all]
Lodash's _.debounce no longer executes callbacks immediately for debounce durations of zero.  Updated tests to reflect this with delays of 0ms to yield and let the debounce fire.

I tried everything I could to get lolex (time simulation library) to work, but lodash stubbornly closes around window.Date so there's not much I can do.
@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster Jul 23, 2016

Member

Note: I opened an issue on lodash/lodash discussing the "breaking" (for us anyway) change to _.debounce in 4.11.1.

Member

islemaster commented Jul 23, 2016

Note: I opened an issue on lodash/lodash discussing the "breaking" (for us anyway) change to _.debounce in 4.11.1.

@@ -1,5 +1,5 @@
<%
var i18n = require('./locale');
var i18n = require('@cdo/netsim/locale');

This comment has been minimized.

@Bjvanminnen

Bjvanminnen Jul 25, 2016

Contributor

Is this related to lodash? Do we have guidance on when to use @cdo vs using a relative path?

@Bjvanminnen

Bjvanminnen Jul 25, 2016

Contributor

Is this related to lodash? Do we have guidance on when to use @cdo vs using a relative path?

This comment has been minimized.

@islemaster

islemaster Jul 25, 2016

Member

This is an approach to including locale files introduced by @pcardune in #9618 (and applied globally in #9621) which lets us stop worrying about require order for locale files in tests. Where before we had to do this:

var utils = require('../../util/testUtils.js');
// We must invoke setupLocales before requiring ModuleUnderTest,
// because it depends on the appropriate localization functions already
// being present on the blockly global.
utils.setupLocales();
var ModuleUnderTest = require('@cdo/apps/ModuleUnderTest');

Now we can just do this, because in tests the locale import within ModuleUnderTest is aliased in tests to a version that does preloading if necessary.

var ModuleUnderTest = require('@cdo/apps/ModuleUnderTest');

This was especially important as we move into ES6-land because Babel hoists import statements (but not require statements), foiling any attempt to do work before an import while using ES6 syntax.

This particular commit makes the same strategy work for NetSim files that don't depend on common locale, only netsim locale. This wasn't causing failures on full test runs because the locale loading in other tests was setting up all locale files, but when using karma:entry to run NetSim tests by themselves (which I was doing while debugging the _.debounce issues) the locale wasn't being loaded.

@islemaster

islemaster Jul 25, 2016

Member

This is an approach to including locale files introduced by @pcardune in #9618 (and applied globally in #9621) which lets us stop worrying about require order for locale files in tests. Where before we had to do this:

var utils = require('../../util/testUtils.js');
// We must invoke setupLocales before requiring ModuleUnderTest,
// because it depends on the appropriate localization functions already
// being present on the blockly global.
utils.setupLocales();
var ModuleUnderTest = require('@cdo/apps/ModuleUnderTest');

Now we can just do this, because in tests the locale import within ModuleUnderTest is aliased in tests to a version that does preloading if necessary.

var ModuleUnderTest = require('@cdo/apps/ModuleUnderTest');

This was especially important as we move into ES6-land because Babel hoists import statements (but not require statements), foiling any attempt to do work before an import while using ES6 syntax.

This particular commit makes the same strategy work for NetSim files that don't depend on common locale, only netsim locale. This wasn't causing failures on full test runs because the locale loading in other tests was setting up all locale files, but when using karma:entry to run NetSim tests by themselves (which I was doing while debugging the _.debounce issues) the locale wasn't being loaded.

This comment has been minimized.

@pcardune

pcardune Jul 25, 2016

Collaborator

I wonder how much value we get out of having separate imports for separate locales used in the different apps folders. Like maybe we should have something like:

import {netsim as i18n} from '@cdo/locale';

We could still keep the actual locale files separate, and just import/export them from the main @cdo/locale module.

@pcardune

pcardune Jul 25, 2016

Collaborator

I wonder how much value we get out of having separate imports for separate locales used in the different apps folders. Like maybe we should have something like:

import {netsim as i18n} from '@cdo/locale';

We could still keep the actual locale files separate, and just import/export them from the main @cdo/locale module.

*/
// make sure Blockly is loaded
require('../frame')();
require('../../../build/package/js/en_us/netsim_locale.js');

This comment has been minimized.

@Bjvanminnen

Bjvanminnen Jul 25, 2016

Contributor

This means npm run test depends on npm run build. Was that the case before?

@Bjvanminnen

Bjvanminnen Jul 25, 2016

Contributor

This means npm run test depends on npm run build. Was that the case before?

This comment has been minimized.

@islemaster

islemaster Jul 25, 2016

Member

It shouldn't; newer:messages (which I believe generates the locale files) is a dependency of the test tasks, and AFAIK it has been for a while.

@islemaster

islemaster Jul 25, 2016

Member

It shouldn't; newer:messages (which I believe generates the locale files) is a dependency of the test tasks, and AFAIK it has been for a while.

This comment has been minimized.

@Bjvanminnen

Bjvanminnen Jul 25, 2016

Contributor

Ahh, nevermind then

@Bjvanminnen

Bjvanminnen Jul 25, 2016

Contributor

Ahh, nevermind then

@Bjvanminnen

This comment has been minimized.

Show comment
Hide comment
@Bjvanminnen

Bjvanminnen Jul 25, 2016

Contributor

lgtm

Contributor

Bjvanminnen commented Jul 25, 2016

lgtm

@islemaster islemaster merged commit 6676650 into staging Jul 25, 2016

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
coverage/coveralls First build on lodash-4-upgrade at 88.009%
Details
hound No violations found. Woof!

@islemaster islemaster deleted the lodash-4-upgrade branch Jul 25, 2016

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