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

Use lodash-amd library. #1299

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Use lodash-amd library. #1299

wants to merge 1 commit into from

Conversation

chrisknoll
Copy link
Collaborator

Fixes #1298

@chrisknoll
Copy link
Collaborator Author

This PR doesn't have any priority to be included in 2.7 (bugfix) release. I saw that there was an AMD lodash library published, and I got a little excited about reducing the footprint.

To minimize code changes around the updated library, what I did was introduced a 'lodash-slim' module which pulls in the distinct set of lodash functions that we reference in Atlas. It turns out, we reference about 35 distinct lodash functions, which is actually a bit surprising to me, and was relieved to find that we weren't using such a large library to call one or two functions, so this library is leveraged quite a bit!

However, I did manage to shave off about 200k (from ~500k) of library size by only including those functions we use.

Ideally, I think the 'proper' way to use the AMD library is to reference the functions where they are required but that left me with 2 problems:

  1. In ohdsi.util (which is a hodge-podge of random functions) there were about 12-15 lodash functions being used, and made the import statements from lodash a bit cumbersome ie: require('lodash/keys'); require('lodash/chain'); etc. It was easier to just create a slim-form of the lodash library, and alias 'lodash' to the slim library via configuration.

  2. I would have had to change all the references to 'lodash' to their specific module paths to get to the function. Similar to problem 1...but where 1 is many functions clumped in 1 place, problem 2 was having to change many files so it was a widespread thing.

So, I decided on the approach in this PR.

Thoughts about version 2.8 or 3.0: We very well could create a separate @ohdsi/lodash module using entirely ES6 classes which exports the required functions from lodash instead of creating the lodash-slim.js module directly in Atlas. This means we could leverage ES6 build tools and optimizations such as 'treeshaking' to get us the smallest possible build (using something like rollup) and then just reference the compiled library via a NPM import, and load it via AMD. What would be nice about that situation is that we wouldn't be loading the individual parts of lodash in the non-bundled mode (the non-bundled mode would request the @ohdsi/lodash module in 1 request)....and I really don't see the need to have to constantly maintain a slim-lodash module since we probably have referenced all the functions we would typically need, and if we needed another from the full offering, we just modify the slim build to include it.

Speaking of 'functions we need', I noticed that there's a few functions in lodash that maybe we could possibly rely on ES6 native methods (like map, reduce, first, find, extend) instead of bundling those functions into the slim library. I understand that there's some differences between the native and the lodash implementation, but my question is more of reducing the footprint.

I welcome your thoughts on this. Thank you.

@anthonysena anthonysena self-requested a review May 14, 2019 18:27
@anthonysena anthonysena self-assigned this May 14, 2019
@anthonysena
Copy link
Collaborator

Moving to 3.0 for consideration in that effort

@anthonysena anthonysena marked this pull request as draft May 12, 2020 12:34
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.

Import lodash-amd v4.71.11
2 participants