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

Revise build system to use gulp #2

Closed
2 tasks done
1egoman opened this issue Oct 16, 2016 · 7 comments
Closed
2 tasks done

Revise build system to use gulp #2

1egoman opened this issue Oct 16, 2016 · 7 comments

Comments

@1egoman
Copy link
Owner

1egoman commented Oct 16, 2016

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
Copy link
Contributor

kme211 commented Oct 16, 2016

Can I work on this?

@1egoman
Copy link
Owner Author

1egoman commented Oct 16, 2016

@kme211 Go for it!

@1egoman 1egoman mentioned this issue Oct 17, 2016
@kme211
Copy link
Contributor

kme211 commented Oct 17, 2016

@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
Copy link
Owner Author

1egoman commented Oct 17, 2016

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
Copy link
Owner Author

1egoman commented Oct 18, 2016

@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 kme211 mentioned this issue Oct 18, 2016
@kme211
Copy link
Contributor

kme211 commented Oct 18, 2016

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
Copy link
Owner Author

1egoman commented Oct 18, 2016

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

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

No branches or pull requests

2 participants