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

Runs npm dedupe before packaging api and sentinel #5732

Merged
merged 4 commits into from Jun 23, 2019
Merged

Conversation

dianabarsan
Copy link
Member

@dianabarsan dianabarsan commented Jun 20, 2019

Description

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.

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
this PR 3.8 MB 2.9 MB 19.0 M

#5739

Code review items

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on medic-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in Changes.md.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@@ -391,7 +391,7 @@ module.exports = function(grunt) {
.map(module =>
[
`cd ${module}`,
`rm -rf node_modules`,
`npm dedupe`,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running dedupe when there are no node_modules ends up generating an empty package-lock, so the next ci errors so beautifully:

TypeError: Cannot read property '@medic/bulk-docs-utils' of undefined at Object.keys.forEach.name 
(/home/diana/.nvm/versions/node/v11.15.0/lib/node_modules/npm/node_modules/lock-verify/index.js:27:40)

npm ci already removes node_modules as its first action.

(cd shared-libs/${lib} && npm ci && npm test)`
)
.join(' && ');
const sharedLibs = getSharedLibDirs();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dedupe actually deletes node_modules from the shared-libs folders, and because some modules require others, testing fails. So we ci all of them first.

'build-common',
'build-node-modules',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching the order is a hack.
Running browserify post dedupe ends up not finding stuff.

@@ -965,7 +963,6 @@ module.exports = function(grunt) {
'install-dependencies',
'static-analysis',
'build',
'build-admin',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already build-admin in our build -> build-common task. It also fails if we attempt to build it at this stage cause browserify doesn't like it when shared libs don't have installed dependencies.

@dianabarsan
Copy link
Member Author

Hey @garethbowen
Can you please have a look and see if it's on the right track? I'll gladly create an issue for this if yes.
Thanks!

Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me.

If browserify is causing issues another way to solve it is to provide aliases in the Gruntfile so you can tell it to look for the node module somewhere else. But if you're happy with this solution that's fine too!

I'll leave it with you to create an issue and prepare for AT :)

@dianabarsan
Copy link
Member Author

@garethbowen
I added the browserify aliases as well, but kept the order hack too.

@garethbowen garethbowen merged commit 6f582c8 into master Jun 23, 2019
@garethbowen garethbowen deleted the test-dedupe branch June 23, 2019 23:22
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