mozilla / oghliner

template and tool for deploying Offline Web Apps to GitHub Pages
https://mozilla.github.io/oghliner/
Apache License 2.0
116 stars 17 forks source link

oghliner.deploy is a footgun #281

Open mykmelez opened 8 years ago

mykmelez commented 8 years ago

oghliner.deploy is a footgun. It doesn't confirm that you've successfully built your app, so it can deploy it in a broken state. I once invoked oghliner deploy after building but not offlining my app, and the deployment appeared to work correctly (the app ran as expected), but since the service worker didn't deploy, the app stopped offlining itself.

If you're using gulp to build, as the template app does, then a naive fix is to make the deploy task depend on the offline task. Since offline depends on build, invoking gulp deploy then ensures that your app will be built and offlined successfully before it's deployed.

But gulp doesn't resolve dependencies across multiple invocations, so if you build and offline in order to test locally (perhaps by running the default task via gulp), and then you gulp deploy, you'll build and offline all over again, which at the very least is unnecessary and also introduces the risk of deploying a different version of the app than the one you tested.

The default .travis.yml file also has this problem, since it invokes gulp to build and then _aftersuccess it invokes gulp deploy to deploy. If deploy depends on offline, then Travis will build twice, once when it invokes gulp, and a second time when it invokes gulp deploy.

We should make oghliner.deploy less of a footgun. I'm not sure exactly what to do, but here are a few ideas:

delapuente commented 8 years ago
  • make oghliner.offline generate a checksum or other artifact from the set of files being offlined, then make oghliner.deploy verify the checksum and fail if it doesn't match the one generated by oghliner.offline.

This is already the case so maybe we could use that as a lock (I'm thinking of adding a .version file with the digest) to stop the offline process to work unnecessary but my concern is if this work we are saving worth the effort as the only remaining step is to write to disk (though it could be reason enough).

mykmelez commented 8 years ago

This is already the case so maybe we could use that as a lock (I'm thinking of adding a .version file with the digest) to stop the offline process to work unnecessary but my concern is if this work we are saving worth the effort as the only remaining step is to write to disk (though it could be reason enough).

It might not be worth the effort merely to avoid re-generating an up-to-date worker on offline. But it is surely worth the effort to avoid deploying an out-of-date worker on deploy!

delapuente commented 8 years ago

Sure, I realized that case later!