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

[TIMOB-24591] Use package-lock.json instead of checking in node_modules #9048

Merged
merged 10 commits into from Jul 20, 2017

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented May 10, 2017

JIRA: https://jira.appcelerator.org/browse/TIMOB-24591

Description:
This migrates us from using npm to manage node_modules dependencies with a checked-in node_modules folder containing production dependencies - to using npm 5+ and a package-lock.json file to set the dependency list to use.

Why?

  • Well for one, we can avoid huge PRs that update npm dependencies checking in thousands of file changes.
  • Smaller clones/faster checkouts/clones. If we stop checking in this large directory tree, our repo size won't be as large and clones/checkouts will be faster. Obviously we have a large history with the folder, so only new clones that are not full (using shallow or depth args) would benefit from it.
  • npm5 does integrity checks, is faster to install (using a local cache), and effectively enforces that everyone is using the same set of dependencies (including development dependencies).

How does this affect me?

  • You need to install npm5+: npm install -g npm@latest
  • Instead of just assuming the node_modules folder exists and has your production dependencies, you'll need to run npm install at the the root of the repo when switching branches. In most cases we don't update dependencies too often, so you could conceivably forget to do this all the time. This is analogous to running npm install when you need to do a local scons/build.
  • If there are updated dependencies, or you've never run npm install and you don't have network access, you could be left with a working copy of the SDK you can't build/test. npm does use a local cache, so if no dependencies have changed and you don't have network access and have blown away your node_modules folder, you could re-run install and have it work.

@sgtcoolguy sgtcoolguy added this to the 6.2.0 milestone May 10, 2017
@cb1kenobi
Copy link
Contributor

I'm cool with this. FR'd just fine.

  1. Question: I see you completely did away with the node_modules/titanium-sdk directory. This is good. I assume you migrated all dependencies on titanium-sdk to node-titanium-sdk?

  2. Note: When I run yarn install, it modifies the yarn.lock file. We should probably advise people to run yarn --pure-lockfile after doing a pull or switching the branch.

  3. Problem: yarn only builds node-ios-device for whatever the current Node.js version/architecture is being used. This won't work. We need to call node_modules/node-ios-device/build-all.sh if node-ios-device changes. We must not commit the node-ios-device binaries to git. I think we may need an install script in the package.json that builds node-ios-device in the dist directory and copies the binaries into the correct place.

@sgtcoolguy
Copy link
Contributor Author

We had already migrated dependencies to use node-titanium-sdk. I moved titanium-sdk to the build directory to be inserted as a shim during the packaging process of the SDK (specifically in build/packager.js). We only need this for backwards compatibility with the titanium-cli's tisdk3fixes hook (which is removed in master).

Yeah, I noticed when I ran a yarn install it slightly modified the yarn.lock for two entries. It just modified the version listing but not the resolved version/URL/checksum. I think because I used yarn import it may be a one-time artifact.

It sounds like we need a package.json install script that simply tells node-ios-device to build all binaries, but I'm not sure why we'd need to do it in the dist directory. Why can't it just build them once when the dev does a yarn install and they live under the module?

I think it does raise an interesting issue we haven't really dealt with before, which is that we build for osx/linux/windows but don't filter the node_modules for the target OS. I had to hack around that with windowslib by removing the os restriction or yarn wouldn't grab it.

Sounds like maybe ioslib/node-ios-device should be optional darwin-only dependencies, windowslib should be optional windows-only - but then how do we handle this on the full build? I can tweak the Jenkinsfile to build on each target OS separately - whereas now it actually runs on OS X and builds each target OS zip (with all the node_modules!).

@cb1kenobi
Copy link
Contributor

I was thinking we would put the node-ios-device binaries in the dist folder since it's git ignored, however I don't think we can do that. There's 2 problems:

  1. the CI server would have to be a macOS machine
  2. I don't want node-ios-device to be compiled by various team members where the Xcode version could be different and thus the binaries would change.

We should add a "os" property to ioslib's package.json and then make ioslib an optional dependency, but then we still need the remaining binaries to be downloaded. This could be as simple as calling node node_modules/node-ios-device/bin/download-all.js (https://github.com/appcelerator/node-ios-device/blob/master/bin/download-all.js).

@cb1kenobi
Copy link
Contributor

On a side note, I have a similar problem with the daemon, so it would be a good idea to have a meeting about how to handle platform-specific native Node addons.

@@ -236,10 +236,22 @@ Packager.prototype.package = function (next) {
cb();
});
}.bind(this),
// Download all the pre-built node-ios-device binaries!
unction (cb) {
Copy link
Contributor

Choose a reason for hiding this comment

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

unction? :)

@sgtcoolguy sgtcoolguy changed the title [TIMOB-24591] Use yarn.lock instead of checking in node_modules [TIMOB-24591] Use package-lock.json instead of checking in node_modules Jun 2, 2017
@sgtcoolguy
Copy link
Contributor Author

There's an issue with node-pre-gyp and npm5: mapbox/node-pre-gyp#298

@sgtcoolguy
Copy link
Contributor Author

Turns out it's an npm5 bug: npm/npm#16926

@sgtcoolguy
Copy link
Contributor Author

So there's an npm5 bug with generating package-lock.json files from an existing set of node_modules, but since all our production dependencies are set to specific versions, it seems Ok to blow away the existing node_modules and run a fresh npm install to get the package-lock.json file.

@sgtcoolguy
Copy link
Contributor Author

Updated the commits to clean up the noise, cherry-picked the changes from #9122 for ioslib 1.4.2

@sgtcoolguy
Copy link
Contributor Author

Note that this PR now contains the same change as #9228 (bumping to windowslib 0.5.2)

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