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

Fails for >=v0.11.9 (--harmony) #437

Closed
rwxrob opened this issue Apr 27, 2014 · 30 comments
Closed

Fails for >=v0.11.9 (--harmony) #437

rwxrob opened this issue Apr 27, 2014 · 30 comments

Comments

@rwxrob
Copy link

rwxrob commented Apr 27, 2014

Working on a personal sample boilerplate for KOAN + Dokku and ran into this annoyance since gulp does not call node --harmony (and probably shouldn't):

SyntaxError: Unexpected token *

(which I'm just including to help with search hits)

#!/usr/bin/env node -> #!/usr/bin/node --harmony fixes it hack-ishly for now, but must be a better way.

Perhaps making gulp happier from the scripts section of package.json would cover it. The following doesn't work but would be nice:

{
  "scripts": {
    "start": "node --harmony server.js",
    "build": "node --harmony gulp"
  }
}
@yocontra
Copy link
Member

Is there any downside to calling with --harmony?

@rwxrob
Copy link
Author

rwxrob commented Apr 27, 2014

Well it's mildly annoying that I have to change the code installed by npm install -g gulp after installation to add --harmony for all projects that use gulp on that system, unless I bite off on local gulp installs or nvm.

As for the other point about adding a build entry to npm, that looks like I failed to fully understand the scripts section. One cannot simply add more and expect them to be executed. I was under the wrong impression there. npm start only works because it is one of the ones that can be defined in package.json.

@rwxrob rwxrob closed this as completed Apr 27, 2014
@heikki
Copy link
Contributor

heikki commented Apr 27, 2014

@robmuh Try npm run-script build.

@rwxrob
Copy link
Author

rwxrob commented Apr 27, 2014

Thanks this worked:

node --harmony node_modules/gulp/bin/gulp.js

which I, of course call now from build as npm run-script build to save typing, lol. I could have just made build call node --harmony node_modules/gulp/bin/gulp.js and listed in build in package.json. Oh the crazy things I'll do to make it just feel right. At least this way someone looking at the sample will get a quick education in what is actually going on without surprises. I don't know how many times I've forgotten --harmony, (remapping alias doesn't quite cut it).

Ya know, I wish there were a way to pass in this and all the other V8 arguments to node from a config file instead of the command line, but I haven't turned anything up in Google yet.

@yocontra
Copy link
Member

@robmuh This is a problem you'll have with any node-based CLI application. I did just find https://github.com/dcodeIO/node-harmonize that we could run if --harmony is detected to relaunch the node process with that flag passed in

@tkellen Does this seem like something you would want to support in liftoff or should this be gulp specific?

@laurelnaiad
Copy link
Contributor

If liftoff could solve this across CLIs that would be awesome. This is a very frustrating issue and it's not just gulp that suffers from it. It would be great to have control over both which node runtime is used and the flags, and to do it based on a config file hierarchy the way liftoff is doing it would be nifty.

@tkellen
Copy link

tkellen commented Apr 27, 2014

Hmm. This feels like something that belongs in Liftoff. I am planning on supporting multiple configuration files in the near future (e.g. with the right config, you could have Liftoff find .gulprc globally, or in some local dir, see gulpjs/liftoff#8). This would at least get us as far as having a unified way to find that sort of configuration.

I'm a little unclear what it would take to support all the v8 flags correctly, though. @robmuh or @stu-salsbury, would either of you be willing to take a stab at implementing a lib to do this? Assuming one existed, I could definitely see Liftoff consuming it.

@yocontra
Copy link
Member

@tkellen We would basically need to make a list of all the node flags and do a check for each, then if any exist respin the process up with them like so https://github.com/dcodeIO/node-harmonize/blob/master/harmonize.js

@yocontra yocontra reopened this Apr 27, 2014
@yocontra yocontra reopened this Apr 27, 2014
@yocontra
Copy link
Member

oops ^

@laurelnaiad
Copy link
Contributor

I happen to be working on a project that is specifically not targeting node 0.11/es6 at the moment, so it would be tough to justify much time right now. However, if a spare moment comes along I can look at. Bottom line, not a great idea to count on me here..

I haven't even looked at how liftoff works... I've just gotten as far as messing with my OS environment in order to get the right node and the right flags in the right situation. If I can clear up some time I'll take a look at how liftoff does its magic and think about how a lib could fit into its use case that would also be more generally useful. The thing about this is that when you multi-version node, you kind of commit to having different global npm databases on your machine, so on a project by project basis you need to manage your npm globals for the runtime that you're intending to use. The developer will need to activate the right npm when they install global module so that it goes to the right place. I'm not sure that there's much that could be done about that part, but I still think there's room for automating the process of selecting a node binary and flags based on liftoff config hierarchy. Whether it belongs in its own module is debatable if the locus for storing the configuration to support it is liftoff configuration.

@rwxrob
Copy link
Author

rwxrob commented Apr 28, 2014

Everything I do -- even production stuff -- is node 0.11, generators are just too great to not use immediately. ;)

However, I am about to start 'summer camp' season (which means almost no side-project time), but I would love to help as much as I am able. I absolutely love gulp and, from the sound of it, the direction for Liftoff.

I looked at harmonize and my first impression was that it won't run on Windows, bash does. I know. Who cares, right? lol. But this will be an issue for many, (just not me since moving our students over to a personal Ubuntu Trusty VM with node v0.11.11 preinstalled, which is also so great for working with Dokku and nginx training).

If nothing more I will follow this thread closely. Thanks again.

@rwxrob
Copy link
Author

rwxrob commented Apr 28, 2014

@contra Also just tried harmonize and found that it messes up the koa and livereload server processes I have in my watch task. Perhaps something that can be worked around, but not worth the hassle at the moment.

@yocontra
Copy link
Member

@robmuh Why wouldn't it run on windows? It just launched a subprocess and proxies stdin/stdout

@rwxrob
Copy link
Author

rwxrob commented Apr 29, 2014

Well just glanced at it and it looked like it was trying to replace the parent with the sub. Whatever the reason, I do know that a perfectly working gulpfile that starts a server.js stops working when I add harmonize. There are so many other, simpler workarounds that I have no problem with them. Thanks for the feedback though.

@tkellen
Copy link

tkellen commented May 9, 2014

http://github.com/tkellen/node-v8flags

Dunno when I'll have time to roll this into Liftoff. PRs welcome!

@tkellen
Copy link

tkellen commented May 9, 2014

Spoke with @tjfontaine. If we can get the v8 flag listing exported from V8 upstream, he'll expose it in node. That'll eliminate needing to scrape for flags the way I am in node-v8flags. Again, not sure when I'll have time for this, just putting it out there.

@cowboy
Copy link

cowboy commented May 9, 2014

FWIW, looked into this for Grunt in the past and came to the conclusion that without a v8 flag listing, it would be a total mess to maintain.

@tkellen
Copy link

tkellen commented May 9, 2014

@cowboy
Copy link

cowboy commented May 9, 2014

You might consider caching the result of that keyed off of process.versions.v8 to avoid extra overhead at runtime.

@tkellen
Copy link

tkellen commented May 9, 2014

Agreed 100%. My plan is to cache it at install time and do a runtime check and throw if the version mismatches, telling the user to npm rebuild (until this is available in node).

@yocontra
Copy link
Member

yocontra commented May 9, 2014

@tkellen You can run this as a preinstall script (serialize list to json file). Any node version changes will require an npm rebuild anyways which will re-run the script. This way you don't have to maintain a list but it's still not done at runtime

@tkellen
Copy link

tkellen commented May 9, 2014

Aye, that's the plan. For now though, I'm hoping the lib being there is enough to motivate someone to implement this on Liftoff. Otherwise this will have to wait a bit--too many other things going atm.

@yocontra
Copy link
Member

yocontra commented Sep 1, 2014

@tkellen Is there an issue on the liftoff repo I can roll this one into?

@yocontra yocontra closed this as completed Sep 1, 2014
@tkellen
Copy link

tkellen commented Sep 2, 2014

There is now. Just spent an hour or two tinkering with a modular solution to this problem--I should have a resolution by the end of the week.

@yocontra
Copy link
Member

yocontra commented Sep 2, 2014

Cool - moving discussion to gulpjs/liftoff#20

@tkellen
Copy link

tkellen commented Sep 4, 2014

The functionality needed to make this work in Liftoff (or any other node binary) is now on npm in these two packages:

If I don't get this integrated into Liftoff before Friday, it'll land sometime late next week.

Am I missing anything here @sindresorhus @phated @contra?
https://github.com/tkellen/node-flagged-respawn/blob/master/index.js#L20-L31

@yocontra
Copy link
Member

yocontra commented Sep 4, 2014

@tkellen A var in front of that child variable 😉

@tkellen
Copy link

tkellen commented Sep 4, 2014

Hah, right you are. I originally had the execute and needed methods together--execute either returned false or the child process. Missed that in the refactor. Fixed!

@tkellen
Copy link

tkellen commented Sep 12, 2014

ref #680

@xpepermint
Copy link

thx @tkellen!

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

7 participants