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

fixed contribution docs #5968

Merged
merged 3 commits into from
Jan 9, 2019
Merged

fixed contribution docs #5968

merged 3 commits into from
Jan 9, 2019

Conversation

mojoaxel
Copy link
Contributor

@mojoaxel mojoaxel commented Jan 8, 2019

This makes it possible to use the local version of gulp (from node_modeues/gulp) to build the library.
The current builds e.g. gulp build of course still work if gulp is globally installed. If gulp is only installed locally via npm install than the user should use npm run gulp to do the same.
This way it is possible to use different versions of gulb for different projects.

I also added the common npm build scripts: npm start and npm test, because they are a common go-to for new developers.

I hope you like these suggestions

@mojoaxel mojoaxel changed the title Buildscripts use locale gulp Jan 8, 2019
@simonbrunel
Copy link
Member

simonbrunel commented Jan 8, 2019

Thanks @mojoaxel for your suggestion.

This way it is possible to use different versions of gulb for different projects.

Only gulp-cli needs to be installed globally, which one use the local gulp defined in package.json. So that's already possible to use different versions of gulp for different projects:

chartjs> gulp --version
[18:01:57] CLI version 2.0.1
[18:01:57] Local version 4.0.0

chartjs-plugin-deferred> gulp --version
[18:05:20] CLI version 2.0.1
[18:05:20] Local version 3.9.1

Actually, the documentation is wrong, it should be:

> npm install
> npm install -g gulp-cli

I also added the common npm build scripts: npm start and npm test

I don't like the idea of mixing npm and gulp commands, it's confusing and at some point we will be asked to duplicate gulp commands to npm, which is useless and require more maintenance. Also, I don't think npm run gulp ... instead of gulp ... is the way we want to promote working in Chart.js sources.

@mojoaxel
Copy link
Contributor Author

mojoaxel commented Jan 8, 2019

Only gulp-cli needs to be install globally

I had a lot of problems getting this to run with my current node (nvm) setup. 😡
Now after cleaning all global modules and reinstalling glob-cli globally it finally works. 😄

To be honest I'm not a big fan of installing things globally, but I guess that is a gulp thing.

I'll fix at least the docs...

I don't like the idea of mixing npm and gulp commands

I see 😐
I'm going to remove the scripts from this PR.

@mojoaxel
Copy link
Contributor Author

mojoaxel commented Jan 8, 2019

Ok, at least I fixed the docs 😉

@mojoaxel mojoaxel changed the title use locale gulp fixed contribution docs Jan 8, 2019
@simonbrunel
Copy link
Member

Thanks @mojoaxel

I'm not fan either of installing stuff globally :) but I think it works pretty well for gulp-cli and I'm not sure it worth trying to get rid of it or creating npm run aliases for every gulp commands (it's also shorter using gulp). And have a partial mix of both doesn't sound great to me.

@simonbrunel simonbrunel added this to the Version 2.8 milestone Jan 9, 2019
@simonbrunel simonbrunel merged commit f342299 into chartjs:master Jan 9, 2019
@simonbrunel
Copy link
Member

Thanks @mojoaxel

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

Successfully merging this pull request may close these issues.

4 participants