1egoman/funnies

Revise build system to use gulp

1egoman opened this issue · 7 comments

1egoman commented

This package's build scripts need some work. I set up the build very fast and I think it's time to move to something like gulp to build this with instead of the mismatch of scripts that are in the package.json.

What this task entails:

  • A super simple Gulpfile:
    • Use babel to transpile react/es2015
    • Bundles into one file with browserify
    • Minifies and removes dead code with uglifyjs
  • Update "main" in the package.json to point to the compiled output.
kme211 commented

Can I work on this?

1egoman commented

@kme211 Go for it!

kme211 commented

@1egoman I have been working on adding Gulp in branch feat-2 in my fork. Can you take a look and let me know if I'm on the right track? https://github.com/kme211/funnies/tree/feat-2

I added a default task which doesn't minify as sort of a development build and then a production task which minfies and and sets the NODE_ENV to production so that Uglifyify will remove the development portion of the React lib.

1egoman commented

Nice job @kme211! A few notes:

  • You should add envify to devdependencies (with something like npm install --save-dev envify)
  • It'd be great if gulp could create a dist folder if it doesn't exist. (Though it's part of the repo in your fork, I've since removed it from upstream so it's likely not to exist in the future.)
  • At some point it'd be great to add a command like gulp watch to this so it can build in the background as one develops. If that's something you want to tackle, go for it!
  • I'm pretty sure that this change breaks compatibility with people using node/browserify to require in the package. The only way I was able to get this to work previously was to output the source files pre-browserify but post-babel to dist/filename.js (so you'd have dist/funnies.js, dist/index.js, etc) and set the main in the package.json to dist/index.js.
  • Lastly, I'd prefer the output files to be called dist/funnies.js and dist/funnies.min.js. The links to these should be updated in the readme as well.

Thanks for all your hard work!

1egoman commented

@kme211 Also, if you open a pull request with your changes Travis CI will start building you changes, which may be helpful to confirm that everything you've written so far works in CI.

kme211 commented

Thanks for taking the time to give me feedback! I think the only thing left is adding a watch task which I'm fine with someone else tackling it since they will probably be able to get to it before I can.

Oh, also I added the babel presets to the dev dependencies but then I realized they were already in the regular dependencies after I submitted my PR.

1egoman commented

(Note to self: issue for a watch task #45)