Upgrade to lodash 4#9684
Conversation
The [`deep` argument of `_.clone` has been removed](https://lodash.com/docs#clone); use `_.cloneDeep` in these cases.
The one-argument version of [`_.zipObject`](https://github.com/lodash/lodash/blob/3.10.1/doc/README.md#_zipobjectprops-values) has become [`_.fromPairs`](https://lodash.com/docs#fromPairs).
The boolean `immediate` option for `_.debounce` has been deprecated, so we should use the `leading` and `trailing` options explicitly.
The [`_.contains`](https://github.com/lodash/lodash/blob/3.10.1/doc/README.md#_includescollection-target-fromindex0) alias has been removed in favor of [`_.includes`](https://lodash.com/docs#includes).
Bind your methods instead
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.
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.
|
Note: I opened an issue on lodash/lodash discussing the "breaking" (for us anyway) change to |
| @@ -1,5 +1,5 @@ | |||
| <% | |||
| var i18n = require('./locale'); | |||
| var i18n = require('@cdo/netsim/locale'); | |||
There was a problem hiding this comment.
Is this related to lodash? Do we have guidance on when to use @cdo vs using a relative path?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
lgtm |
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
deepargument to_.clone()has been removed; we now use_.cloneDeep()instead of_.clone(_, true)._.zipObject()(accepting data in the format[['key', 'value'], ['key', 'value']]) has been moved to a new method_.fromPairs._.debouncewent through several changes; see below._.containsalias for_.includeshas been removed._.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 (likeFunction.prototype.bindor arrow functions).What's up with
_.debounce?A few things: The official breaking change in the 4.0 upgrade is that
_.debounceno longer accepts a boolean as its third argument. Way back in Lo-Dash (sic) 1.0.0 debounce's third argument was just a booleanimmediatethat 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,
_.debouncehas been touched a ton since v4.0.0, sometimes in breaking ways:_.debounceand_.throttle."maxWaittimeout isn’t processed on a leading call ifleadingis false & there isn’t a max delay queued"._.debounceto simplify, reduce timers, & fix bugs"_.debouncedoesn’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 likesetTimeout(func, 0)and doesn't invoke until the next tick. I updated my tests to expect this (basically, doingsetTimeout(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.