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

Added npm build script for building with local grunt. #643

Merged
merged 1 commit into from
Feb 6, 2018

Conversation

jvmccarthy
Copy link
Contributor

@jvmccarthy jvmccarthy commented Feb 2, 2018

@pablojim thank you for making and sharing highcharts-ng! I have two motivations for this PR,

  1. I ran into the issue fixed in Browser loading bug #592 and request that the fix be published to npm (perhaps as version 1.2.1)
  2. Because 1.2.1-dev wasn't rebuilt (the dist folder still had 1.2.0), I forked the repo to create a build. I don't have grunt installed globally, preferring npm scripts these day because they can keep dependencies local. This is a pattern adopted by many since npm scripts will fix up the path to allow for local dependencies and for conventional targets like npm run build. Just looking at some other examples, angular.js uses a script wrapper for the grunt command in their package.json and vue.js uses a build script wrapper in their package.json. Personally, I favor a build script over a grunt script but that's preference. Since you have a default build target in your gruntfile, you have the added advantage of being able to provide a build target by simply wrapping grunt. For example, npm run build -- watch will run the equivalent of grunt watch.

Feel free to close this PR. The main motivation is the need for a new version. But, I do think npm scripts are a popular option these days as well.

Thank you for your time and consideration.

@pablojim pablojim merged commit 96bac54 into pablojim:master Feb 6, 2018
@pablojim
Copy link
Owner

pablojim commented Feb 6, 2018

Thanks @jvmccarthy I'll try get to a release shortly.

@jvmccarthy jvmccarthy deleted the npm-build-script branch February 6, 2018 17:41
@prols prols mentioned this pull request May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants