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

Added Bundle Node Module Support #54

Merged
merged 5 commits into from
May 28, 2019
Merged

Conversation

erisu
Copy link
Member

@erisu erisu commented May 14, 2019

Platforms affected

electron

Motivation and Context

Support Bundling Node Modules

fixes: #52

Description

This PR is a possible solution to support the bundling of node_modules inside the Electron built package.

The concept is that when working on an app, we add specific npm packages (npm i -S <package>) to the root directory of the Cordova project as a dependencies, not devDependencies.

For example, if we wanted to use lodash:

  1. Config the Cordova Project to support nodeIntegration.

Follow these guides:

  1. Install the lodash npm node modules.
npm i -S lodash

Now what happens?:

  • During testing with the cordova run electron --nobuild command, it will spawn an Electron process and use the lodash modules from <project_root>/node_modules/lodash.

    This is how it operated before this PR.

  • When building, the Cordova build process will copy the Cordova's project dependencies from package.json to the built application's package.json. During the build process, it will npm install all the modules within the app directory that is packaged.

    This was added from this PR.

Note: With the current Cordova CLI, all platforms are added as dependencies. IT SHOULD BE ok to manually move these dependencies to devDependencies. These dependencies contain only the build tools and templates and are not needed in your bundled app. All it will do is increase the application's size.

Testing

  • npm t

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@brodycj
Copy link

brodycj commented May 14, 2019

I would personally favor an approach that works the same way across all platforms.

For example, we could use a tool such as Babel, Webpack, or Typescript to transform the sources and combine them into a bundle for each platform.

P.S. Thanks to @erisu for your efforts on this one.

@erisu
Copy link
Member Author

erisu commented May 14, 2019

@brodybits we are talking about bundling all node modules that can also include executable binaries.

@brody
Copy link

brody commented May 15, 2019

@erisu I think you meant @brodybits 🙈

@erisu
Copy link
Member Author

erisu commented May 15, 2019

Sorry @brody, you are correct. I was typing this on my phone 🙈 Thank you.

Copy link
Contributor

@GedasGa GedasGa left a comment

Choose a reason for hiding this comment

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

Look good to me. However, I was wondering, maybe the DOCUMENTATION.md file should be submitted as a separate PR.

@erisu erisu marked this pull request as ready for review May 24, 2019 06:12
@tiptronic
Copy link

As soon as this is merged I am volunteering to see if it works :) -> but that's a big improvment!
Thx @erisu

@codecov-io
Copy link

codecov-io commented May 24, 2019

Codecov Report

Merging #54 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #54   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          14     14           
  Lines         605    612    +7     
=====================================
+ Hits          605    612    +7
Impacted Files Coverage Δ
bin/templates/cordova/lib/prepare.js 100% <100%> (ø) ⬆️
bin/templates/cordova/lib/PackageJsonParser.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 639b0c7...f447b37. Read the comment docs.

@janpio
Copy link
Member

janpio commented May 24, 2019

PR sounds awesome.

Note: With the current Cordova CLI, all platforms are added as dependencies. IT SHOULD BE ok to manually move these dependencies to devDependencies. These dependencies contain only the build tools and templates and are not needed in your bundled app. All it will do is increase the application's size.

Should this be changed in Cordova CLI?

However, I was wondering, maybe the DOCUMENTATION.md file should be submitted as a separate PR.

Where did the documentation for this feature end up?

@erisu
Copy link
Member Author

erisu commented May 24, 2019

@janpio

Note: With the current Cordova CLI, all platforms are added as dependencies. IT SHOULD BE ok to manually move these dependencies to devDependencies. These dependencies contain only the build tools and templates and are not needed in your bundled app. All it will do is increase the application's size.

Should this be changed in Cordova CLI?

Yes, the long-term solution on how Cordova install platforms and plugins though CLI will need changing within CLI/fetch. I recall from a conversation with others that this type of change would be considered major but also desirable.

This is why I mention the manual process for an immediate solution while we work on the long-term.

However, I was wondering, maybe the DOCUMENTATION.md file should be submitted as a separate PR.

Where did the documentation for this feature end up?

The documentation he was referring to that was in this PR was not exactly only for this feature. There was an example that explains how to enable nodeIntegration, but the example had a mistake. Though nodeIntegration is partly related and needed, I went ahead and commit it under an update PR that I had been working on anyways.

As an end note on documentation, maybe I can add within this PR a doc update specifically talking about the feature strictly.

@erisu erisu modified the milestone: 1.1.0 May 27, 2019
@erisu erisu requested a review from janpio May 27, 2019 08:36
package-lock.json Outdated Show resolved Hide resolved
@erisu erisu merged commit 506553c into apache:master May 28, 2019
@erisu erisu deleted the bundle-node-modules branch June 17, 2019 03:49
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.

Bundle node_modules
7 participants