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

Dependency upgrade? #3466

Closed
megakoresh opened this issue Jan 7, 2016 · 14 comments
Closed

Dependency upgrade? #3466

megakoresh opened this issue Jan 7, 2016 · 14 comments
Labels
more info please npm install issue An issue running `npm install`, whether that's inside of a Sails app, or with `npm install sails -g`

Comments

@megakoresh
Copy link

Hi, I don't really know what all that fuss about Express 4 was (not even gonna link it), but I have recently been very frustrated with how out of date many of Sails dependencies are(particularly lodash). And I am not sure sticking to those old versions is actually necessary.

For example I forced lodash v3.1.0 and everything worked totally fine (using sails-azuretables and sails-disk adapters). And when you install Sails it always complains about deprecated dependencies. It does make people wary when I show the project to them and makes me uncomfortable too.

Would it really be that much work to make sure up to date dependencies are used?
FYI here are packages that npm complains about when installing v0.12-rc5:
region

  • lodash (wants v3.x.x)
  • grunt-lib-contrib (was split into separate modules)
  • win-spawn (wants node-cross-spawn instead)

I am sure there are others, those are just the most visible. Also does anyone know if this framework is going to be upgraded to ES2015 at any point? Seems like everyone is doing it these days, so I figured I should ask.

@japel
Copy link

japel commented Jan 7, 2016

@megakoresh I somewhat agree with you... but then again not... dependency management is no easy task, and although something seems to work at first glance, it does not mean that it really works as espected, unless there are tests that cover it.
But then, again, the longer you wait the more challenging it gets to update to the latest stable version of a package, specially if there were big api changes.
I think many of these packages will be upgraded because of this:
#3464

@megakoresh
Copy link
Author

Some already have. I just want to bring this issue to light so to speak.

@japel
Copy link

japel commented Jan 7, 2016

👍

@mikermcneil
Copy link
Member

Hi @megakoresh- sorry to hear you've been frustrated by the deprecation messages in the logs when you install Sails in a new project. We've spent a considerable amount of effort running our automated tests and performing manual tests (lifting existing apps and being sure Grunt picks up changes, deleted files, etc.) and locked our dependencies to versions that we consider production-ready.

If you are able to put together a set of updated dependency versions which pass our tests and solve those problems when running manual tests, I'd be more than happy to take a look. Thanks!

btw- re: express 4, I've recently posted a more detailed explanation of our approach here.

@mikermcneil mikermcneil added the npm install issue An issue running `npm install`, whether that's inside of a Sails app, or with `npm install sails -g` label Jan 8, 2016
@sgress454
Copy link
Member

Re: dependencies in general, see my comment on this thread (and the entire thread if you feel like reading a long discussion about dependencies).

Upgrading Lodash actually breaks Sails, but that's been the case for a while now, so I'm going to look into fixing it so we can move to a newer version.

@japel
Copy link

japel commented Jan 9, 2016

Damn those lodash devs!!!!11eleven

:)

@megakoresh
Copy link
Author

Well I forked the repo and tried to run your tests without any dependency changes and it failed anyway:
windows powershell

This happens regardless of whether I run npm test from root of the sails repo or test folder.

@sgress454
Copy link
Member

@megakoresh Hm, what version of Windows/Node/NPM 3 are you on? Tests pass a-okay here and on Travis for Node 0.10, 0.12, 4 and 5.

Actually, after looking into this a bit, it looks like it might be a problem where your Node.exe isn't associated with .js files. See nodejs/node-v0.x-archive#9338.

@megakoresh
Copy link
Author

Node 5.1.0
NPM 3.3.6

@mikermcneil
Copy link
Member

@megakoresh thanks! What version of Windows are you using?

@megakoresh
Copy link
Author

Windows 10. I will see about that registry key thing, but I don't think that is the problem because I can manually do the whole ./bin/sails.js -bla -bla -bla thing and it is recognized as a command.

UPDATE: Yeah, I edited the key into the registry (wasn't there by default for some reason), and got exactly the same result as before.

@sgress454
Copy link
Member

Thanks @megakoresh. I looked into this some and it looks like those tests are not Windows-friendly (using / in paths instead of path.sep or just path.resolve for the whole thing). I only have Windows 8 to test with but it looked like the exact same issue. I'll hack on this some more today--we'd certainly like our Windows brethren to be able to contribute!

@sgress454
Copy link
Member

I'm going to open a separate issue for fixing the tests on Windows.

As for the original issue here, except for win-spawn, all of those dependency warnings are coming from sub-dependencies that we're using the latest versions of (things like grunt-contrib-jst). It's definitely worth looking at those projects and pinging the maintainers to see if they can bump their deps, but it's not something we can handle from our end (except again for win-spawn, which I'll look into while fixing the tests for Windows).

@megakoresh
Copy link
Author

In that case I will try to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more info please npm install issue An issue running `npm install`, whether that's inside of a Sails app, or with `npm install sails -g`
Development

No branches or pull requests

4 participants