menpo / landmarker.io

Image and mesh annotation web application
https://www.landmarker.io
BSD 3-Clause "New" or "Revised" License
114 stars 21 forks source link

Move to webpack, incremental builds, remove gulp #121

Closed jabooth closed 8 years ago

jabooth commented 8 years ago

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

jabooth commented 8 years ago

@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 commented 8 years ago

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.

lirsacc commented 8 years ago

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 commented 8 years ago

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.

jabooth commented 8 years ago

@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?