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

A request to move @airbnb/node-memwatch to devDependencies in package.json #257

Closed
ColinBradley opened this issue Aug 23, 2018 · 1 comment

Comments

@ColinBradley
Copy link

Any chance of @airbnb/node-memwatch being moved into a devDependency in package.json? As it's just increasing CI times for us - especially sad as it is a dev dependency. It also causes errors that get ignored in some environments (not a real bother or your problem).
I have resorted to using the --no-optional flag for npm install and npm prune and that does avoid the problem. Using that flag feels a little heavy handed for just your package though.

Although this isn't your problem, the errors I get look like:

..\src\memwatch.cc(19): fatal error C1083: Cannot open include file: 'sys/time.h': No such file or directory [...\node_modules\@airbnb\node-memwatch\build\memwatch.vcxproj]
..\src\heapdiff.cc(291): warning C4244: 'argument': conversion from 'unsigned __int64' to 'v8::SnapshotObjectId', possible loss of data [...\node_modules\@airbnb\node-memwatch\build\memwatch.vcxproj]
..\src\heapdiff.cc(303): warning C4244: 'argument': conversion from 'unsigned __int64' to 'v8::SnapshotObjectId', possible loss of data [...\node_modules\@airbnb\node-memwatch\build\memwatch.vcxproj]
..\node_modules\nan\nan_new.h(208): warning C4244: 'argument': conversion from 'unsigned __int64' to 'double', possible loss of data (compiling source file ..\src\heapdiff.cc) [...\node_modules\@airbnb\node-memwatch\build\memwatch.vcxproj]

I understand that you might not want dev package restores to fail, but increasing everyone else's bother isn't great either.

Anyway, just a humble request :)

@rochdev
Copy link
Member

rochdev commented Aug 23, 2018

That was actually a mistake on my part. I moved it from devDependencies to optionalDependencies because it doesn't install on Node <8 but I forgot that optionalDependencies are not installed only in dev. I'll simply remove the dependency completely and install it as a build step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants