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

Make bundles smaller by deduping dependencies #5739

Closed
dianabarsan opened this issue Jun 21, 2019 · 18 comments
Closed

Make bundles smaller by deduping dependencies #5739

dianabarsan opened this issue Jun 21, 2019 · 18 comments
Assignees
Labels
Priority: 1 - High Blocks the next release. Type: Technical issue Improve something that users won't notice
Projects

Comments

@dianabarsan
Copy link
Member

Post #5694 (which made our shared-libs local again), our api and sentinel bundles doubled in size because of lots of duplicated dependencies ( npm pack straight up copies the symlinked folders ).
Running npm dedupe reduces our bundle sizes to what they were before.

Version api sentinel compiled.json
3.5.x 3.7 MB 2.9 MB 20.1 MB
master 7.9 MB 6.5 MB 19.0 M
with dedupe 3.8 MB 2.9 MB 19.0 M
@dianabarsan dianabarsan added Type: Technical issue Improve something that users won't notice Priority: 1 - High Blocks the next release. Type: Improvement Make something better labels Jun 21, 2019
@dianabarsan dianabarsan self-assigned this Jun 21, 2019
@dianabarsan dianabarsan added this to To do in 3.6.0 via automation Jun 21, 2019
@dianabarsan
Copy link
Member Author

Ready for AT on test-dedupe (sorry about the name).

@dianabarsan dianabarsan moved this from To do to In AT in 3.6.0 Jun 21, 2019
@garethbowen garethbowen removed the Type: Improvement Make something better label Jun 23, 2019
garethbowen pushed a commit that referenced this issue Jun 23, 2019
Post #5694 our api and sentinel bundled doubled in size because of lots of duplicated dependencies (`npm pack` straight up copies the symlinked folders ).
Running `npm dedupe` reduces our bundle sizes to what they were before.

#5739
@garethbowen
Copy link
Member

I've merged this into master to unblock some other work. So you can now AT directly against master.

@dianabarsan
Copy link
Member Author

I have another branch with a different and, hopefully, more stable approach: 5739-with-links.

@tookam
Copy link
Contributor

tookam commented Jul 1, 2019

@dianabarsan I am unable to build because of the following:

Running "browserify:webapp" (browserify) task
>> Error: Cannot find module '/projects/medic-webapp/webapp/node_modules/@medic/phone-number' from '/projects/medic-webapp'

@dianabarsan
Copy link
Member Author

@wtekeu
How are you running the build - which grunt task are you using locally?

@tookam
Copy link
Contributor

tookam commented Jul 1, 2019

@dianabarsan grunt build

@dianabarsan
Copy link
Member Author

dianabarsan commented Jul 1, 2019

@wtekeu
In this case, yea, you didn't run the task that would also link your dependencies.

grunt build does the following tasks:

grunt.registerTask('build', 'Build the static resources', [
    'exec:clean-build-dir',
    'copy:ddocs',
    'build-common',
    'build-node-modules',
    'minify',
    'couch-compile:primary',
  ]);

If you run dev-webapp, you should get the dependencies too. Could you try running that?

@tookam
Copy link
Contributor

tookam commented Jul 1, 2019

@dianabarsan the command ln -sr failed on on my macOS.

>> added 317 packages in 8.582s
>> ln: illegal option -- r
>> usage: ln [-Ffhinsv] source_file [target_file]
>>        ln [-Ffhinsv] source_file ... target_dir
>>        link source_file target_file
>> Exited with code: 1.
>> Error executing child process: Error: Process exited with code 1.
Warning: Task "exec:npm-ci-modules" failed. Use --force to continue.

@dianabarsan
Copy link
Member Author

Ok, I'll look into it.

@ngaruko ngaruko moved this from In AT to In progress in 3.6.0 Jul 1, 2019
@dianabarsan
Copy link
Member Author

@wtekeu I changed the options on ln, it should now work on macOS as well. Could you, please, check again?

@dianabarsan dianabarsan moved this from In progress to In AT in 3.6.0 Jul 2, 2019
@tookam
Copy link
Contributor

tookam commented Jul 2, 2019

@dianabarsan Now it is build (grunt build) that is failing:

Running "exec:pack-node-modules" (exec) task
>> npm
>> WARN prepare removing existing node_modules/ before installation
>> added 219 packages in 2.699s
>> npm ERR!
>> path /Users/wtekeu/projects/codes/medic-webapp/api/node_modules/@medic/transitions/node_modules/.bin/mustache
>> npm
>> 
>> ERR! code EEXIST
>> npm
>> ERR! Refusing to delete /Users/wtekeu/projects/codes/medic-webapp/api/node_modules/@medic/transitions/node_modules/.bin/mustache: is outside /Users/wtekeu/projects/codes/medic-webapp/api/node_modules/@medic/transitions/node_modules/mustache and not a link
>> npm
>> ERR! File exists: /Users/wtekeu/projects/codes/medic-webapp/api/node_modules/@medic/transitions/node_modules/.bin/mustache
>> npm
>> ERR! Move it away, and try again.
>> 
>> npm ERR! A complete log of this run can be found in:
>> npm ERR!     /Users/wtekeu/.npm/_logs/2019-07-02T17_52_06_993Z-debug.log
>> Exited with code: 1.
>> Error executing child process: Error: Process exited with code 1.

I have tried running npm dedupe inside api and I get the same error message:

npm ERR! path /Users/wtekeu/projects/codes/medic-webapp/api/node_modules/@medic/transitions/node_modules/.bin/mustache
npm ERR! code EEXIST
npm ERR! Refusing to delete /Users/wtekeu/projects/codes/medic-webapp/api/node_modules/@medic/transitions/node_modules/.bin/mustache: is outside /Users/wtekeu/projects/codes/medic-webapp/api/node_modules/@medic/transitions/node_modules/mustache and not a link
npm ERR! File exists: /Users/wtekeu/projects/codes/medic-webapp/api/node_modules/@medic/transitions/node_modules/.bin/mustache
npm ERR! Move it away, and try again.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/wtekeu/.npm/_logs/2019-07-02T17_53_54_930Z-debug.log

I have tried deleting api/node_modules, it did not fix it.

@dianabarsan
Copy link
Member Author

Which command are you running? again grunt build stand-alone? which command did you run before it?

@tookam
Copy link
Contributor

tookam commented Jul 2, 2019

grunt 'dev-webapp'

@dianabarsan
Copy link
Member Author

dianabarsan commented Jul 2, 2019

Which npm version are you using?
I see plenty of people reporting this bug.

@tookam
Copy link
Contributor

tookam commented Jul 2, 2019

6.4.1

@dianabarsan
Copy link
Member Author

Pff thanks. I did try it with this version locally and didn't have this issue, might be some peculiarity. I'll give it another try tomorrow and reach out if I can't replicate.

@dianabarsan
Copy link
Member Author

I ran grunt dev-webapp followed by grunt build using node@11 and npm@6.4.1 and it works correctly, with no errors.
I'm seeing that someone reports they got this error because some symlinks became regular files: https://stackoverflow.com/a/47795071/1865409
I'm wondering if, in MacOS, cp creates copies the symlinked files instead of copying the symlink itself.

@tookam
Copy link
Contributor

tookam commented Jul 3, 2019

LGTM. Ready for release. @dianabarsan will merge.

3.4.x

-rw-r--r--   1 wtekeu  staff   3.8M Jul  3 09:35 medic-api-0.1.0.tgz
-rw-r--r--   1 wtekeu  staff   3.0M Jul  3 09:35 medic-sentinel-0.1.0.tgz
-rw-r--r--  1 wtekeu  staff    19M Jul  3 09:36 compiled.json

5739-with-links-fs-extra

-rw-r--r--   1 wtekeu  staff   3.8M Jul  3 09:24 medic-api-0.1.0.tgz
-rw-r--r--   1 wtekeu  staff   2.9M Jul  3 09:24 medic-sentinel-0.1.0.tgz
-rw-r--r--  1 wtekeu  staff    19M Jul  3 09:24 compiled.json

@tookam tookam closed this as completed Jul 3, 2019
3.6.0 automation moved this from In AT to Done Jul 3, 2019
kennsippell pushed a commit that referenced this issue Jul 5, 2019
Post #5694 our api and sentinel bundled doubled in size because of lots of duplicated dependencies (`npm pack` straight up copies the symlinked folders ).
Running `npm dedupe` reduces our bundle sizes to what they were before.

#5739
dianabarsan added a commit that referenced this issue Jul 10, 2019
An alternative to depending on npm to correctly map local scoped dependencies.
We do not add our shared libraries as dependencies in our package.json files any more.
For development, the libs are symlinked into node_modules by grunt - like npm ci would do.
For creating packages, both api and sentinel have a new property in their package.json that should list the shared-libs that the module uses and must be bundled.

#5739
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 1 - High Blocks the next release. Type: Technical issue Improve something that users won't notice
Projects
No open projects
3.6.0
  
Done
Development

No branches or pull requests

3 participants