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

Move to webpack, incremental builds, remove gulp #121

Merged
merged 26 commits into from
Nov 21, 2015
Merged

Move to webpack, incremental builds, remove gulp #121

merged 26 commits into from
Nov 21, 2015

Conversation

jabooth
Copy link
Member

@jabooth jabooth commented Oct 26, 2015

Build times were getting pretty long (~8 seconds) since adopting ES2015 and Babel. Watchify is a browserify extension to enable incremental builds - I originally intended to issue this PR with a move to watchify. However, I ran into issues getting things set up, and figured that it might be interesting to look at an alternative that is gaining in popularity - webpack.

It only took me a few hours to get a working setup and I have to say I'm impressed. Incremental builds are now very fast (~300 ms), and our gulp setup is much simplified. It also leaves open the option of hot module reloading for React Components down the line if my experiments with React are successful...

In part of this change, I've removed gulp from the project entirely. Instead, we just rely on simple npm scripts for builds.

To try:

> npm install
> nam run watch
> open http://localhost:8080/webpack-dev-server/ in browser

note that webpack includes a builtin server for development that dynamically reloads after building.

Changes

This moves all top-level root static content into a ./static dir, to keep things tidy.

Deployment now looks like:

  1. Delete ./build if it exists
  2. Copy all of ./static into ./build
  3. Bundle JS/SASS/img assets from ./src and ./img into ./build with webpack. Watch for changes on all assets and rebuild when needed.

Now the whole website as it should be served at https://www.landmarker.io is stored at build. This means we will need to change our deployment scripts to account for this. Also for now I've disabled App Cache for development at least. We will have to think through re-enabling it for production builds, and I guess in light of landmarker.app whether this is even something we want any more.
App Cache is enabled still for production builds only.

TODO

  • export img to build (placeholder not working)
  • create production gulp command that builds without dev server
  • update deploy.sh to use ./build (maybe we stop using gh-pages here?)
  • update s3 website config to copy from ./build not ./
  • see if autoprefixer needs to be re-enabled (may require webpack plugin)
  • re-enable App Cache (only for production)

@jabooth
Copy link
Member Author

jabooth commented Oct 26, 2015

@lirsacc, want to check this out if you have time? I think you'll like the change :)
However, it does need some thought in terms of how it impacts our deployment. If you have any thoughts I'd love to hear them!

@jabooth
Copy link
Member Author

jabooth commented Oct 27, 2015

For AppCache, we can just have it for production builds. We don't actually need the cache busting has hes on assets anyway - the app cache itself has a cache busting string, and that is all that is required. For dev builds that's one less pain point to worry about, and for production nothing has to change.

jabooth added a commit that referenced this pull request Oct 28, 2015
Ignore build and static dirs (see #121)
@lirsacc
Copy link
Contributor

lirsacc commented Oct 28, 2015

I am checking it out now, and first thing I see: why keep both webpack and gulp ? The orchestration can be done in the webpack file (js) and thus we could remove the gulp dependencies. Might give it a go while I am going over the changes.

For the deployment, as long as we keep putting all the same files in the correct folder it should keep up transparently.

Regarding Github pages, that is still how we support mixed content and local servers (insecure domain), so removing it from deploy means dropping support ?

@jabooth
Copy link
Member Author

jabooth commented Oct 28, 2015

I am checking it out now, and first thing I see: why keep both webpack and gulp ? The orchestration can be done in the webpack file (js) and thus we could remove the gulp dependencies.

This is true, we only really need string replacement for adding the App Cache on production and something to copy the static content over to the build dir.

Saying that, I like the separation for the time being - webpack.config.js just records how we need to bundle our assets together. Stuff like actually running a development server (with the different settings that are required), cleaning the build dir etc aren't really concerns of the pack generation, so they seem at home for me in the gulpfile.js. We only have a few gulp deps now, don't know if it's really worth it?

Regarding Github pages, that is still how we support mixed content and local servers (insecure domain), so removing it from deploy means dropping support ?

That is true, and it's something we can't really drop support for now. I think in this PR it's better to leave things as is with gh-pages, and just switch over to webpack with minimal changes. If we decide to commit to AWS fully, that can be a separate piece of work.

I've got some other comments for stuff I wasn't 100% sure on, I'll add them in the relevant places.

@@ -99,7 +99,8 @@ ls "staging/$BRANCH"
# tags are deployed at root
if [[ ! -z "$TRAVIS_TAG" ]]; then
echo "Deploying tag $TRAVIS_TAG"
rm -fr ./index.html ./bundle-*.* ./*.appcache ./img ./api
# clean out everything that's non gh-pages specific
ls | grep -vw 'v1\|legacy\|staging\|CNAME' | xargs rm -r
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lirsacc I now delete everything in ./ bar except a blacklist rather than selectively delete items - needed as webpack files will generally have different names

@jabooth
Copy link
Member Author

jabooth commented Oct 29, 2015

@lirsacc I've fixed the minor issues I highlighted - I now believe this to be correct but untested.

For now I think I would prefer to leave the webpack.config.js as purely a declaration of the webpack configuration - I don't have any problem with the three remaining gulp deps - unless you strongly feel otherwise?

Do you have any advice on how to test deploy.sh other than going for it with a tagged release and being on standby to address any issues manually?

@jabooth jabooth changed the title Move to webpack, incremental builds Move to webpack, incremental builds, remove gulp Nov 21, 2015
jabooth added a commit that referenced this pull request Nov 21, 2015
Move to webpack, incremental builds, remove gulp
@jabooth jabooth merged commit 87fe140 into master Nov 21, 2015
@jabooth jabooth deleted the webpack branch November 25, 2015 13:58
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