upgrade lodash - Githubissues

pixelzoom commented 9 years ago

lodash doc indicates that some functions have been renamed. Eg contains -> includes. Rather than continuing to use the old functions, we should probably upgrade. As of this writing, lodash latest is 3.10.1, PhET is using 2.4.1.

Various useful links:

Project page: https://lodash.com GitHub: https://github.com/lodash/lodash Changelog: https://github.com/lodash/lodash/wiki/Changelog CDN: https://www.jsdelivr.com/projects/lodash API doc: https://lodash.com/docs

samreid commented 8 years ago

Newer lodash also has a nice _.endsWith function that @andrewadare and I would like to use, it's another good reason to upgrade.

It is unclear whether we should upgrade to lodash "core" or "full", they differ by about a size of 4x.

It is also unclear how much testing will be necessary, or if there are API changes that will be incompatible (if backward compatibility is broken).

pixelzoom commented 8 years ago

A Google search of "lodash backward compatibility" recommends looking at the changelog, https://github.com/lodash/lodash/wiki/Changelog. The most obvious breaking changes can be identified by searching for "Renamed" and "Removed". I'm sure there are others.

pixelzoom commented 8 years ago

The lastest version of lodash is 4.13.1, were's using 2.4.1.

samreid commented 8 years ago

@pixelzoom said it might make sense to add StringUtils.endsWith

pixelzoom commented 8 years ago

... using the implementation from this polyfill: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/endsWith

samreid commented 8 years ago

In the above commit, StringUtils obtained endsWith from MDN polyfill as well as unit tests to cover it's basic usage.

samreid commented 8 years ago

I'm not opposed to updating lodash but I think StringUtils is a better place for endsWith, and I like using the MDN implementation.

mattpen commented 8 years ago

The lodash-migrate package may be useful https://www.npmjs.com/package/lodash-migrate

pixelzoom commented 8 years ago

7/14/16 dev meeting: Assigned to me. First thing I'll do is survey what we're currently using. Try to use core vs full package.

pixelzoom commented 8 years ago

I wrote a script to find (and count) uses of lodash functions, see lodashFind.sh.

Here's the current output:

% lodashFind.sh 1048 extend 300 each 102 random 58 map 54 times 54 contains 52 indexOf 32 max 31 filter 30 clone 26 shuffle 25 sortBy 25 pull 24 forEach 22 keys 21 reduce 17 some 14 find 13 min 12 range 10 every 9 uniq 8 without 8 pick 8 omit 7 isFinite 7 after 6 isUndefined 6 flatten 6 any 4 remove 4 isNumber 4 isEqual 4 first 3 size 3 last 3 isEmpty 3 identity 3 defaults 2 values 2 unique 2 union 2 sample 2 noop 2 lastIndexOf 2 foldl 2 difference 2 debounce 2 constant 1 zip 1 tail 1 isNaN 1 isFunction 1 isArray 1 intersection 1 inject 1 groupBy 1 forOwn 1 findIndex 1 extends 1 defined 1 chain

pixelzoom commented 8 years ago

According to https://github.com/lodash/lodash/wiki/build-differences, the core build is designed to be Backbone ≥ v1.3.0 compatible, is 4kB gzipped, and contains only 64 methods (sic). Four of PhET's top six (extend, random, times, contains) are not included in core, so we're not going to be able to use the core build.

pixelzoom commented 8 years ago

There is a significant size difference between core and full builds. https://lodash.com says:

Core build (~4 kB gzipped) Full build (~22 kB gzipped)

The version that PhET is currently using (sherpa/lib/lodash-2.4.1.js) is reportedly a custom build. But it is 28kB minified, which is larger than the full build. So I'm wondering if it really is a custom build.

Custom build commands and options are described at https://lodash.com/custom-builds. Does anyone know who created the custom build, what commands & options were used, etc?

pixelzoom commented 8 years ago

Disadvantages of using a custom build:

(1) If you want to use a lodash function that you haven't used previously, consulting the lodash documentation is insufficient. You will also need to consult the source in sherpa/lib/ before using a function, to see if it was included in the custom build.

(2) If a function that you want to use is not included in the custom build, you'll need to create a new custom build if you want to use that function. This will be a significant barrier to using lodash to its full potential.

So my recommendation would be to use the full build.

pixelzoom commented 8 years ago

I downloaded the various pre-built versions (4.13.1) that are available from https://www.jsdelivr.com/projects/lodash, and here's what I see:

% ls -lh | awk '{print $5 " " $9}' 112K lodash.core.js 12K lodash.core.min.js 25K lodash.fp.js 9.4K lodash.fp.min.js 504K lodash.js 66K lodash.min.js 10K mapping.fp.js

The 2 files that would replace what's currently in sherpa/lib/ are:

504K lodash.js 66K lodash.min.js

Compare sizes to what's currently in sherpa:

224K lodash-2.4.1.js 28K lodash-2.4.1.min.js

Labeling for discussion at developer meeting.

pixelzoom commented 8 years ago

7/21/16 dev meeting: Consensus is to use full build.

samreid commented 8 years ago

Apparently _.extend is deprecated in the new lodash. It has been renamed to _.assignIn

it also looks like _.assign() may be appropriate for our use cases.

I'll create a separate issue for this.

pixelzoom commented 8 years ago

Looking more closely at the [lodash changelog]()... For major releases, they have sections titled "Compatibility Warnings" and "Low Risk Compatibility Warnings".

PhET is currently using v2.4.1, so the warnings for v3.0.0 and v.4.0.0 are relevant:

v3.0.0: https://github.com/lodash/lodash/wiki/Changelog#v300 v4.0.0: https://github.com/lodash/lodash/wiki/Changelog#v400

There is definitely mention of functions used by PhET in the warnings, so I'm expecting there will be breakable, some of it possibly subtle/odd.

pixelzoom commented 8 years ago

In the middle of the upgrade to 4.13.1, I discovered that 4.14.0 was released 2 days ago. The CDN does not currently have built versions available, and I think that it makes sense to wait. Especially since the 4.14.0 release notes say:

• Optimized .endsWith, .negate, & _.startsWith

... and a desire to use _.endsWith (https://github.com/phetsims/sherpa/issues/58#issuecomment-232391230) was what restarted this issue.

samreid commented 8 years ago

True, but we now have StringUtils.endsWith with the MDN implementation.

pixelzoom commented 8 years ago

I asked about availability of 4.14.0 on the CDN in https://github.com/lodash/lodash/issues/2544, and was told that it's "already there". It still doesn't show up on the "CDN copies" link provided on the lodash project page, and it's not a browser caching issue, maybe a jsdelivr issue. Anyway... My question did get me direct links. Here are the direct links to the deliverables that PhET needs:

https://cdn.jsdelivr.net/lodash/4.14.0/lodash.js https://cdn.jsdelivr.net/lodash/4.14.0/lodash-min.js

pixelzoom commented 8 years ago

4.14.1 has been released and is available via the CDN, https://www.jsdelivr.com/projects/lodash

pixelzoom commented 8 years ago

Deferring this until after my vacation.

samreid commented 8 years ago

Four of PhET's top six (extend, random, times, contains) are not included in core, so we're not going to be able to use the core build.

How did you conclude that lodash.core dosen't contain extend? If I click on lodash core build from their homepage, it shows me this file: https://raw.githubusercontent.com/lodash/lodash/4.15.0/dist/lodash.core.js

which includes the line:

lodash.extend = assignIn;

Hence it looks like lodash.core contains extend. I also tested in the browser and confirmed that _.extend exists in the core.

We will no longer be using _.random. I confirmed that times and contains are not in the core build.

I was re-investigating this because after looking at many of lodash's method signatures in https://github.com/phetsims/dot/issues/58 , I'm becoming aware how many of the methods have flexible arguments. This can lead to odd behavior as identified in the dot issue, but also disables browser optimizations and increases complexity for tasks for which PhET is often only using a very simple subset.

Here's the top of our list of used methods and what is in core is checked:

[x] 1048 extend
[x] 300 each
[ ] 102 random (we will no longer be using this)
[x] 58 map
[ ] 54 times (this would be trivial to reimplement)
[ ] 54 contains (this would be trivial to reimplement)
[x] 52 indexOf
[x] 32 max
[x] 31 filter
[x] 30 clone
[ ] 26 shuffle (we will no longer be using this)
[x] 25 sortBy
[ ] 25 pull
[x] 24 forEach
[x] 22 keys
[x] 21 reduce
[x] 17 some
[x] 14 find
[x] 13 min
[ ] 12 range
[x] 10 every
[ ] 9 uniq
[ ] 8 without
[x] 8 pick
[ ] 8 omit
[x] 7 isFinite
[ ] 7 after
[x] 6 isUndefined
[x] 6 flatten

pixelzoom commented 8 years ago

@samreid asked:

How did you conclude that lodash.core dosen't contain extend?

Based on the documentation at https://github.com/lodash/lodash/wiki/build-differences.

Regardless, we concluded (after much discussion) that requiring PhET developers to stick to a subset of lodash was undesirable.

pixelzoom commented 8 years ago

@samreid Are you proposing that we revisit the issue of whether to use core vs full build? Or should I proceed with updating to a full build?

pixelzoom commented 8 years ago

@samreid said:

I was re-investigating this because after looking at many of lodash's method signatures in phetsims/dot#58 , I'm becoming aware how many of the methods have flexible arguments. This can lead to odd behavior as identified in the dot issue, but also disables browser optimizations and increases complexity for tasks for which PhET is often only using a very simple subset.

Or perhaps you're proposing that we ditch lodash altogether and implement the functions that we need?

samreid commented 8 years ago

Based on the documentation at https://github.com/lodash/lodash/wiki/build-differences.

The documentation indicates that lodash.core has _.assignIn, which is the alias for _.extend

Regardless, we concluded (after much discussion) that requiring PhET developers to stick to a subset of lodash was undesirable.

We may have come to the wrong conclusion based in part on incorrect assumptions.

Our decision was based in part on the assumption that lodash.core was missing _.extend, which is incorrect. Choosing lodash.min (71KB) over lodash-core.min (12KB) means choosing 59KB of bloat for every file op: building sims, uploading sims, disk space, downloading sims, inspecting minified files, etc. Rutherford Scattering is 1466 KB, so in this case it amounts to 4% bloat. If today some PhET developer found a way to cut all of our sims down by 4% we would be very happy. Hence I think we should revisit this decision.

pixelzoom commented 8 years ago

@samreid said:

Our decision was based in part on the assumption that lodash.core was missing _.extend, which is incorrect.

Yes, in part. But we also said that expecting developers to know which functions are included in core was unreasonable. And the minute that someone wanted something that's not in core (e.g. endsWith, which started this process), then we'd be regretting the decision to save a few kilobytes.

I will put this issue on hold, and label for (re)discussion at developer meeting.

samreid commented 8 years ago

Consensus: let's continue with full for now. 59KB is not too worrisome, and we can avoid the work paring down to core, and the overhead of having to remember what is in core vs full, and mitigating risk of later wishing we had something in core.

pixelzoom commented 8 years ago

@ariel-phet Based on current priorities, I'm unlikely to get to this anytime soon. If you want to leave it assigned to me, that's fine. If you want it done sometime in the next 2-3 months, you should probably assign it to someone else.

ariel-phet commented 8 years ago

Marking for dev meeting, I want to get a sense for the team's feeling of the priority of this upgrade (but likely will have someone take it over from @pixelzoom)

samreid commented 8 years ago

@jbphet will put it on his calendar to remind us in a month or so.

samreid commented 8 years ago

Oops, accidentally closed, reopening.

samreid commented 8 years ago

In https://github.com/phetsims/sherpa/issues/65#issuecomment-249918315, @andrewadare said:

I have been assigned to test .assign, but this is blocked by #58. Un-assigning myself for now; please re-assign me if/when .assign should be tested in a new lodash library version.

jbphet commented 8 years ago

My reminder went off to mark this for developer meeting.

mattpen commented 8 years ago

Deferring discussion till next week.

jessegreenberg commented 8 years ago

10/27/16 - We will revisit this issue in mid-december, @jbphet will create a reminder for us to revisit.

jbphet commented 7 years ago

My reminder went off, and I've labeled this for discussion at developer meeting.

jbphet commented 7 years ago

Discussed in the 12/15/2016 dev meeting, we're hitting the snooze button on this until early Feb.

samreid commented 7 years ago

Welcome to early February! In developing CCK, I could use lodash _.minBy() but it looks like it was introduced in 4.0.0. It seems we are still using 2.4.1.

pixelzoom commented 7 years ago

@samreid said:

In developing CCK, I could use lodash _.minBy() ...

Example?

samreid commented 7 years ago

ElectronPropagator.js could read:

    chooseCircuitElement: function( circuitLocations ) {
      var self = this;

      // Choose the circuit location with the lowest density
      var minBY = _.minBy( circuitLocations, function( circuitLocation ) {
        return circuitLocation.getDensity( self.circuit );
      } );

samreid commented 7 years ago

I'm going to look into a lodash upgrade now.

samreid commented 7 years ago

I'm running into https://github.com/mgonto/restangular/issues/1298

jonathanolson commented 7 years ago

We could switch to _.includes?

samreid commented 7 years ago

Agreed, I'm testing that out in my working copy.

samreid commented 7 years ago

Initial signs are promising:

I'll run a full aqua.

samreid commented 7 years ago

.contains, .any and _.unique are all missing.

samreid commented 7 years ago

Testing complete, let's see what we have:

Here's a URL with the fails: http://localhost/aqua/test-server/test-sims.html?ea&audioVolume=0&testDuration=10000&testConcurrentBuilds=4&fuzzMouse&brand=phet&testSims=circuit-construction-kit-black-box-study,circuit-construction-kit-dc,color-vision,energy-forms-and-changes,energy-skate-park-basics,gene-expression-essentials,gravity-and-orbits,molarity,molecule-shapes,molecule-shapes-basics,molecules-and-light,neuron,reactants-products-and-leftovers,states-of-matter-basics,trig-tour,under-pressure

Here's the error log: Sim errors (dev):

gene-expression-essentials Running "eslint:allFiles" (eslint) task

/Users/samreid/github/gene-expression-essentials/js/common/model/attachmentstatemachines/AttachedToBasePair.js 132:101 error Unnecessary semicolon no-extra-semi

✖ 1 problem (1 error, 0 warnings)

Warning: Task "eslint:allFiles" failed. Use --force to continue.

Aborted due to warnings.

samreid commented 7 years ago

4.0.0 removed these aliases:

https://github.com/lodash/lodash/wiki/Roadmap

.all, .any, .backflow, .callback, .collect, .compose, .contains, .detect, .foldl, .foldr, .include, .inject, .methods, .object, #run, .select, & _.unique

phetsims / sherpa

upgrade lodash #58