search

Kitware / minerva

Minerva: client/server/services for analysis and visualization
Apache License 2.0
36 stars 14 forks source link

Upgrade geojs to v0.8.0 #332

Closed mgrauer closed 8 years ago

mgrauer commented 8 years ago

Ready for review. Fixes #318.

I had a bit of trouble installing this, I ended up removing my girder/node_modules dir and girder/plugins/minerva/node_modules dir, then running npm install under the girder dir and everything was fine.

jbeezley commented 8 years ago

I'm having similar troubles... The grunt tasks are clearly borked. I've tried running things like grunt --debug-js default watch, and it does something different each time. Sometimes it runs the build first, sometimes it doesn't start the watch task, sometimes the watch task mysteriously stops working... I'm ready for webpack now.

jbeezley commented 8 years ago

Yeah this is an unfortunate state of affairs. If you delete plugins/minerva/node_modules, but not node_modules, then the build fails with 

Running "shell:minerva-geojs-build" (shell) task

> geojs@0.8.0 build /Users/jbeezley/git/minerva/node_modules/geojs
> grunt init library

>> Local Npm module "grunt-contrib-jade" not found. Is it installed?
>> Local Npm module "grunt-template" not found. Is it installed?
>> Local Npm module "grunt-contrib-copy" not found. Is it installed?
>> Local Npm module "grunt-contrib-uglify" not found. Is it installed?
>> Local Npm module "grunt-contrib-watch" not found. Is it installed?
>> Local Npm module "grunt-contrib-clean" not found. Is it installed?
>> Local Npm module "grunt-contrib-concat" not found. Is it installed?
>> Local Npm module "grunt-express" not found. Is it installed?
>> Local Npm module "grunt-docco" not found. Is it installed?
Warning: Task "uglify:ext" not found. Use --force to continue.

until you delete both node_modules directories. It looks like the path resolution is getting seriously confused by some combination of the nested node_modules directories and the symlinks.

I think it's caused by my changes in #321. We could either revert the changes to the Gruntfile in that PR, or just get rid of the geojs build step entirely and just use the version on npm. I'm more inclined to the later as we can always cut a new geojs release if it is needed.

mgrauer commented 8 years ago

I'll change to pulling from NPM.

mgrauer commented 8 years ago

@jbeezley

Now from npm.

jbeezley commented 8 years ago

@mgrauer

I removed the geojs build step as well to get rid of the problem we were seeing. If it LGTY, it LGTM.

mgrauer commented 8 years ago

@jbeezley

I think your change to pull pnltri from minerva/node_modules was a typo (it broke for me when I tried), I've fixed it here.

Also, it seems like you are using shell:plugin-minerva as a base dependency for the minerva init tasks, so I followed your lead.

LGTM. LGTY?

jbeezley commented 8 years ago

I think your change to pull pnltri from minerva/node_modules was a typo (it broke for me when I tried), I've fixed it here.

No, it wasn't. Now that you mention it, this is actually a backwards incompatible difference between npm 2 and 3. For npm 2, the path to the library will be minerva/node_modules/geojs/node_modules/pnltri. For npm 3, it will be minerva/node_modules/pnltri. I think the best thing to do would be to add pnltri as a direct dependency of minerva that way it will always be in minerva/node_modules/pnltri.

Also, it seems like you are using shell:plugin-minerva as a base dependency for the minerva init tasks, so I followed your lead.

Yes, shell:plugin-minerva is the task that runs npm install locally, so it is required for anything using files in minerva/node_modules.

mgrauer commented 8 years ago

For npm 3, it will be minerva/node_modules/pnltri

I'm on npm v 3.4.1. When I tested your commit f21e252 , I had deleted girder/node_modules and minerva/node_modules, before running an npm install under girder. That installed pnltri under minerva/node_modules/geojs/node_modules and pnltri wasn't available to the client.

I'm good with adding pnltri directly as a minerva dependency, though now we'll need to track pnltri versions to keep up with geojs (this is far from our worst problem). I'll add a commit with "pnltri": "^2.1.1".

mgrauer commented 8 years ago

@jbeezley

This worked for me.

jbeezley commented 8 years ago

I'm on npm v 3.4.1. When I tested your commit f21e252 , I had deleted girder/node_modules and minerva/node_modules, before running an npm install under girder. That installed pnltri under minerva/node_modules/geojs/node_modules and pnltri wasn't available to the client.

Welp, ¯\_(ツ)_/¯. Pnltri is probably going away as a geojs dep anyway. This LGTM.

mgrauer commented 8 years ago

¯\_(ツ)_/¯ is the new LGTM.