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

Move to modular lodash #244

Merged
merged 3 commits into from
Dec 9, 2018
Merged

Move to modular lodash #244

merged 3 commits into from
Dec 9, 2018

Conversation

mwaldstein
Copy link
Contributor

  • Removed use of chain as it required (effectively) all of lodash
  • Switched to native Function.bind for the same reason.

With similar change to graphlib, dagrejs/graphlib#86

  • dagre.js: 636K => 317K
  • dagre.min.js: 554K => 278K

 * Removed use of `chain` as it required (effectively) all of lodash
 * Switched to native Function.bind for the same reason.

With similar change to graphlib,

 * dagre.js: 636K => 317K
 * dagre.min.js: 554K => 278K
Need updated dist files for CI test
@mwaldstein
Copy link
Contributor Author

Some notes -

This is more invasive than changes to graphlib or dagre-d3. The reason is that 2 lodash functions effectively ended up pulling in or needing all of lodash, negating the exercise. These functions were used in limited places, so they were written out.

_.chain

Permits composing functions rather than nesting - e.g.

var result = ._chain(input).function1().function2().value()

vs

var result = function2(function1(input))

used in lib/order/cross-count.js and lib/order/resolve-conflicts.js

In both cases there were only 2 functions in the chain, so while nesting is less attractive, it is not illegible.

No risk.

_.bind

The lodash bind is fairly powerful with a lot of options. dagre only uses it to set a scope for 'this' in lib/position/bk.js

There is a low risk with this change. lodash uses Function.prototype.apply to create its bind which has been supported in browsers forever. I made the change to use the native Function.prototype.bind which does not go back as far - mainly not supported by ie8 (compatibility)

There are options to add a polyfill if ie8 support is desired.

@mwaldstein
Copy link
Contributor Author

Follow-up - graphlib is already using Function.prototype.bind, so ie8 is already non-functional.

@lutzroeder
Copy link
Contributor

The dagre.js built via mwaldstein:master seems to be larger than dagre:master. Any ideas?

@mwaldstein
Copy link
Contributor Author

I would imagine it is because of using the unmodified graphlib.

Without also making this change dagrejs/graphlib#86, you end up still with the full lodash since graphlib gets fully bundled in.

@lutzroeder lutzroeder merged commit f60d1a0 into dagrejs:master Dec 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants