yeoman / yeoman-app

A desktop app that scaffolds projects using Yeoman
988 stars 106 forks source link

Added npm debug scripts #94

Closed ruyadorno closed 8 years ago

ruyadorno commented 8 years ago

What do you think about this @stefanbuck ?

I imagine you were probably already using node-inspector (or something similar) on your side to debug the browser process but I think it would be a good idea to have these tasks declared as npm scripts and standardize the debug process.

My idea is to have the two tasks, that will need to be run in parallel:

stefanbuck commented 8 years ago

I like the idea but some comments from my side:

  1. We should declare node-inspector as a peerDependencies
  2. We can not ensure that port 5858 is always free, right?
  3. We should also write it down in our dev docs (Maybe this is already enough, so we don't have to care about the other points).

Didn't know that pre and post commands are working with custom scripts. Thanks for teaching me something new every PR :tada:

ruyadorno commented 8 years ago

:smile: hahaha, I haven't even thought about it, just assumed the pre and post would work everywhere

  1. makes sense but I'm still not convinced, we got to keep in mind that peerDependencies do not auto-install anymore - it would make sense to have it as a devDependencies since it's actually part of the development, on the bright side it's more convenient to just run npm install and have a complete working environment
  2. nope... we got to pick a port though, I prefer to stick with the defaults if there's no reason to do otherwise... anyways, we can also declare the port number on electron --debug=5858 if we want to be more explicit about it
  3. sure thing! let's decide on the first point and I'll update it :blush:
stefanbuck commented 8 years ago

I know that peerDependencies are not auto install anymore, but for me it's still a peerDependency rather than a devDependency. The user will receive a warning in the console if the peerDependency is not installed so he could perform the install command by himself.

Anyway, if we adding node-inspector as a devDependency we have to exclude it from the build https://github.com/yeoman/yeoman-app/blob/e4ebc74969941b67520f085716aff3e39c467a2c/scripts/build.js#L29-L30 to reduce the app size. Finally, my favorite argument, each new dependency slows down the CI build and the initial npm install command :stuck_out_tongue:

ruyadorno commented 8 years ago

hehehe :+1: agreed, those are good arguments - specially thinking that not everyone might actually want to debug

I'll work on these changes

stefanbuck commented 8 years ago

specially thinking that not everyone might actually want to debug

Yes, that's the best one.

ruyadorno commented 8 years ago

:+1: done!

stefanbuck commented 8 years ago

:tada: Thanks for adding this!