Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

[WIP] feat(core): use npm commands #1296

Merged
merged 3 commits into from
Jul 9, 2016
Merged

Conversation

simison
Copy link
Member

@simison simison commented Apr 5, 2016

Replace variety of gulp/grunt/sh/npm commands with npm commands.

Todo:

  • Add commands
  • Test
  • Test with Docker
  • Update readme

See #1258 for discussion.

Replace variety of commands by npm commands.

See meanjs#1258
@simison
Copy link
Member Author

simison commented Apr 5, 2016

Notes:

"test:watch:onlyChanged": "gulp test:server:watch --onlyChanged"

npm commands do support parameters, but it's kinda cumbersome (npm command -- --param). Actually I think we could get rid of the parameter, see: #1297

"prestart": "if [ ! -d node_modules ]; then npm install --quiet; fi"

So no need to run npm install, just clone and npm start will do. Thoughts?
Edit: removed.

"postinstall": "bower install --allow-root --config.interactive=false && bower prune --allow-root --config.interactive=false"

--allow-root helps with docker and CI environments. Note that it doesn't turn sudo on, it merely allows it. Normally Bower would complain about it. This sorta just moves responsibility to developer, assuming they should know what they're doing with user rights. Thoughts? These config vars could of course be moved to .bowerrc but I think here they're more clear.
Edit: moved interactive to bowerrc.

@codydaig codydaig added this to the 0.5.0 milestone Apr 5, 2016
"test:e2e": "gulp test:e2e",
"test:watch": "gulp test:server:watch",
"test:watch:onlyChanged": "gulp test:server:watch --onlyChanged",
"prestart": "if [ ! -d node_modules ]; then npm install --quiet; fi",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too black box magic for me, I don't think we should do this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilanbiala All the commands in general or just the prestart command?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prestart

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed to it being in there, but don't see any reasons to have it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like people are gonna ask questions about it and it's gonna confuse beginners more than help.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simison Go ahead and remove this command. There's really no benefit to using this vs. npm install. Plus, this doesn't cover the edge case where a module was added or failed to install and npm install needs to be ran again even though node_modules directory exists.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codydaig 👍 done!

@ilanbiala valid point. It's a boilerplate project after all.

@codydaig codydaig self-assigned this Apr 6, 2016
@coveralls
Copy link

Coverage Status

Coverage remained the same at 70.615% when pulling 24eac7d on simison:npm-commands into bc0b4a6 on meanjs:master.

@ilanbiala
Copy link
Member

Also, can we move the bower flags into a bowerrc?

@simison
Copy link
Member Author

simison commented Apr 6, 2016

Also, can we move the bower flags into a bowerrc?

Done.

Looks like I can't put --allow-root there, tho. http://bower.io/docs/config/

@coveralls
Copy link

Coverage Status

Coverage remained the same at 70.615% when pulling 761e217 on simison:npm-commands into bc0b4a6 on meanjs:master.

@ilanbiala
Copy link
Member

Looks a lot cleaner to me 👍

@trendzetter
Copy link
Contributor

👍
also wasn't there the new gulp-wiredep or something?

@mleanos
Copy link
Member

mleanos commented Apr 29, 2016

We could simplify the scripts section in the package.json by only including the tasks necessary for install, testing, and deployment; we don't need all the tasks that are solely used for development. IMO, the only testing task we need is test.

If we add a script entry of "gulp": "gulp" (uses the locally installed version of Gulp), we could run any other Gulp task defined in gulpfile.js with npm run gulp test:server:watch. This could be very useful for our users, since they may have a list of custom tasks that they would want to continue to use. If a user prefers to use Grunt, or a mix of Gulp & Grunt, then they could add an entry for Grunt to the scripts section.

@mleanos
Copy link
Member

mleanos commented May 15, 2016

@simison Any thoughts on my previous comment?

@codydaig
Copy link
Member

Bump. Any progress with this PR?

@lirantal
Copy link
Member

@codydaig I definitely want to see this through! I'll get back to it if @simison won't be continuing this PR.

@mleanos
Copy link
Member

mleanos commented Jul 6, 2016

@codydaig @lirantal Any thoughts on my comment?

@codydaig
Copy link
Member

codydaig commented Jul 6, 2016

@mleanos IMO, I would like to see the most common tasks put in the scripts section of package.json (ie. watch). Tasks that are only typically ran ocassionally like seeding of the database, can be left to the gulp/grunt file.

@lirantal
Copy link
Member

lirantal commented Jul 7, 2016

@codydaig , guys this LGTM and I want to merge it in with a follow-up PR for updating the README and docs. I don't think we need to "simplify" it. It's not complex if it is documented and we get people used to use the npm command.

@mleanos I get what you mean with making the gulp task more portable. I don't think it conflicts, we can also add that kind of command to the script and win both worlds.

WDYS?

@mleanos
Copy link
Member

mleanos commented Jul 8, 2016

@lirantal That sounds good to me. Perhaps we could add the entry for Gulp, with the follow up PR with an explanation of this in the docs.

In that case, this PR LGTM.

@simison
Copy link
Member Author

simison commented Jul 10, 2016

Excellent, thanks @codydaig and @mleanos for taking care of this! I'm super busy myself right now.

@mleanos FYI, this changed since this PR: eaead7a

@mleanos
Copy link
Member

mleanos commented Jul 11, 2016

@simison Do you see the commit that you referenced as a conflict, or an issue?

From my perspective neither this commit, or the changes to the test:server:watch Gulp task have an impact over the other. The task name hasn't changed, but the internal task used within the Gulp config has; this should be OK though.

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

Successfully merging this pull request may close these issues.

7 participants