Skip to content
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

Polyfills for IE11 / PhantomJS #6087

Closed
26 tasks done
bhousel opened this issue Mar 22, 2019 · 4 comments
Closed
26 tasks done

Polyfills for IE11 / PhantomJS #6087

bhousel opened this issue Mar 22, 2019 · 4 comments
Assignees
Labels
performance Optimizing for speed and efficiency
Milestone

Comments

@bhousel
Copy link
Member

bhousel commented Mar 22, 2019

We really need to start weaning our way off Lodash.

I'm not providing the before/after flamecharts, but this change in coreGraph made a substantial performance improvement in Chrome:

// this.entities = _assign(Object.create(base.entities), other.entities);
// this._parentWays = _assign(Object.create(base.parentWays), other._parentWays);
// this._parentRels = _assign(Object.create(base.parentRels), other._parentRels);
this.entities = Object.assign(Object.create(base.entities), other.entities);
this._parentWays = Object.assign(Object.create(base.parentWays), other._parentWays);
this._parentRels = Object.assign(Object.create(base.parentRels), other._parentRels);

All modern browsers support Object.assign, but we have used Lodash's _assign to support legacy browsers like IE11 and PhantomJS (which runs our tests).

I'm going to use this ticket to list specific things that we'll remove from the iD code and replace with browser-native equivalents. No we're not going full ES6 or introducing Babel transpilation, so don't ask for that.

  • _.assign() -> Object.assign
  • _.clone() -> Object.assign({} , thing) or Array.slice
  • _.compact()-> Array.filter(Boolean)
  • _.extend() -> Object.assign (subtly different from _.assign())
  • _.every() -> Array.every
  • _.filter() -> Array.filter
  • _.find() -> Array.find
  • _.findIndex() -> Array.findIndex
  • _.includes() -> Array.indexOf !== -1
  • _.indexOf -> Array.indexOf
  • _.isFunction -> typeof thing === 'function'
  • _.keys() -> Object.keys
  • _.omit() -> Object.keys(obj).reduce trick - do this
  • _.pull() -> Array.filter (like remove, and mutate array)
  • _.reject() -> Array.filter (with inverted predicate)
  • _.remove() -> Array.filter (with inverted predicate, and mutate the array)
  • _.some() -> Array.some
  • _.values() -> Object.values
  • _.without() -> Array.filter (like remove but return a new array, which filter does)

Array helpers can be implemented with Set - do this

  • _.difference()
  • _.intersection()
  • _.union()
  • _.uniq()
    and add util functions to replace
  • _.chunk()
  • _.groupBy()
  • _.uniqBy()
@bhousel bhousel added the performance Optimizing for speed and efficiency label Mar 22, 2019
@bhousel
Copy link
Member Author

bhousel commented Mar 22, 2019

I'm using the browser-polyfills which seems to have everything we need.

  • Polyfills provided by browser-polyfills
    • Promise
    • fetch
    • Map
    • Set
    • Array.find
    • Array.findIndex
    • Array.from
    • Object.values
    • Object.assign
    • requestAnimationFrame (rAF)

bhousel added a commit that referenced this issue Mar 22, 2019
bhousel added a commit that referenced this issue Mar 23, 2019
bhousel added a commit that referenced this issue Mar 23, 2019
bhousel added a commit that referenced this issue Mar 23, 2019
bhousel added a commit that referenced this issue Mar 23, 2019
bhousel added a commit that referenced this issue Mar 23, 2019
bhousel added a commit that referenced this issue Mar 24, 2019
bhousel added a commit that referenced this issue Mar 24, 2019
(re: #6087)

I ran into some trouble here, it looks like `Array.from()` polyfill
doesn't work for Sets in PhantomJS
@bhousel
Copy link
Member Author

bhousel commented Mar 25, 2019

Just a heads up that Array.from(Set) doesn't seem to work under PhanomJS 😢
I might put some hacky code into test/spec_helpers.js to work around it.

I ended up doing this, but I don't want to do it everywhere

iD/modules/core/history.js

Lines 316 to 332 in 0c7d986

var s = new Set();
_stack.slice(1, _index + 1).forEach(function(state) {
state.imageryUsed.forEach(function(source) {
if (source !== 'Custom') {
s.add(source);
}
});
});
if (window.mocha) {
var arr = [];
s.forEach(function(v) { arr.push(v); });
return arr;
} else {
return Array.from(s);
}
// var arr = _map(_stack.slice(1, _index + 1), 'imageryUsed');
// return _without(_uniq(_flatten(arr)), 'Custom');

@bhousel bhousel self-assigned this Mar 25, 2019
bhousel added a commit that referenced this issue Mar 25, 2019
#6087 (comment)

Also added `npm run phantom` to test workarounds like this quickly
bhousel added a commit that referenced this issue Mar 26, 2019
bhousel added a commit that referenced this issue Mar 26, 2019
bhousel added a commit that referenced this issue Mar 26, 2019
bhousel added a commit that referenced this issue Mar 27, 2019
(closes #6116)

This is breakage related to #6087 in replacing _.uniqBy with a Set
The lodash functions are much more tolerant of nulls and dodgy inputs.
bhousel added a commit that referenced this issue Mar 27, 2019
(closes #6092)

This is breakage related to #6087 in replacing lodash `_.filter`
(which can iterate (v,k) over an object) with `Array.filter`
bhousel added a commit that referenced this issue Mar 27, 2019
@bhousel
Copy link
Member Author

bhousel commented Mar 28, 2019

I've been working on this for a few days and it's really nice to have our lodash usage trimmed down significantly.

Here's what's left:

  • _.isEqual(), _.isMatch()
    These are used for deep object comparisons.. need to think of how/whether to replace.

  • _.map(), _.reduce(), _.forEach(), _.forOwn()
    These are used to iterate over objects, and can be removed case-by-case

  • _.flatten(), _.flattenDeep()
    Might keep, remove, rewrite.. decide case-by-case.

  • _.cloneDeep()
    Deep cloning is mostly easy with JSON.parse(JSON.stringify(thing)) (except that it fails on things like Dates, Functions, Set/Map etc) but it's also possible to write a much more performant version depending on the thing being deepcloned.

  • _.debounce(), _.throttle()
    These are useful and we should keep these, or maybe switch to a different non-lodash dependency.
    Update: I tried swapping these calls out and using throttle-debounce but that library seems not as mature, and this was causing problems with cancelled calls. We can leave these alone, since they don't pull in a lot of lodash and they aren't used for hot code.

bhousel added a commit that referenced this issue Mar 28, 2019
bhousel added a commit that referenced this issue Mar 29, 2019
(re: #6087)

Tests in Phantom run twice as fast now!
bhousel added a commit that referenced this issue Mar 29, 2019
(re #6087)

- some were able to do a different approach
    (validations/almost_junction, and settings/*)
- some were replaced with custom speedy cloners
   (in orthogonalize clonePoints and osm.js cloneNoteCache)
- some just replaced with JSON.parse(JSON.stringify()))
bhousel added a commit that referenced this issue Mar 30, 2019
bhousel added a commit that referenced this issue Mar 30, 2019
@bhousel
Copy link
Member Author

bhousel commented Mar 30, 2019

This was done! 😅

To recap:

  • performance improvements throughout iD, but particularly in graph/difference
    (basically, every edit you make)
  • test time cut in half
  • we can use the browser polyfills now for a lot of ES6 things
  • lodash is mostly gone now, except for debounce and throttle, this is good because
    • Rollup has trouble tree shaking just the pieces of it we used
    • Modern browser engines like V8 don't like to optimize it

Screenshot 2019-03-30 10 01 28

Most of iD's large bundle size now is because of the data that we bundle into iD. This includes: the presets (particularly the name-suggestion-index which is big now), the imagery index, the community index, the English strings, and the intro walkthrough data (the biggest chunk on that chart). These are all things we can load separately once we focus on splitting iD into small modules for iD v3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Optimizing for speed and efficiency
Projects
None yet
Development

No branches or pull requests

2 participants