search

pablojim / highcharts-ng

AngularJS directive for Highcharts
MIT License
1.73k stars 463 forks source link

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

Closed jvmccarthy closed 6 years ago

jvmccarthy commented 6 years ago

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

  1. I ran into the issue fixed in #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 commented 6 years ago

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