Skip to content
This repository has been archived by the owner on Nov 27, 2018. It is now read-only.

[WIP] Offliner integration #96

Closed
wants to merge 9 commits into from

Conversation

delapuente
Copy link
Contributor

@mykmelez do you want to take a look, please?

@delapuente delapuente changed the title Offliner integration [WIP] Offliner integration Oct 16, 2015
@@ -16,6 +16,8 @@

'use strict';

var Promise = require("es6-promise").Promise;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we require Node 0.12 now, Promise should be available already, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, confirmed with @brendandahl

@delapuente
Copy link
Contributor Author

Hi @mykmelez , can you give me a second opinion about the changes?

@digitarald
Copy link

@marco-c could you check this while @mykmelez is out?

// it in the list of files to cache.
try {
fs.unlinkSync(path.join(rootDir, 'offline-worker.js'));
fs.unlinkSync(path.join(rootDir, 'offliner-worker.js'));
Copy link
Contributor

Choose a reason for hiding this comment

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

This change should also be reverted.

Adding new templates to integrate offliner.
Adding the sw template as well.
Changing bootstrap to ignore the sw template and fix Promise import.
Promise import fixed for configure as well.
es6-promise added to package, added blob as well and removed the use of
sw-precache.
lib/offline changed to fill the sw template.
Lazy loading offliner-client.js and safeguard.js
Offliner included without minifying the files for enabling further optimizations.
Modifying the update strategy to reinstall() as soon as the worker has been updated.
Modifying the manager to handle updates by reloading the application.
@delapuente
Copy link
Contributor Author

Hi @mykmelez , what about another round, please?

gutil.log(gutil.colors.red.bold(file + ' is bigger than 2 MiB. Are you sure you want to cache it? To suppress this warning, explicitly include the file in the fileGlobs list.'));
}
function flatGlobs(fileGlobs) {
return Object.keys(fileGlobs.reduce(function (matchings, fileGlob) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: matchings -> matches

@mykmelez
Copy link
Contributor

mykmelez commented Nov 5, 2015

Two other issues:

  1. The integrate command copies scripts/offline-manager.js into the app and tells the user to load it: https://github.com/mozilla/oghliner/blob/master/lib/integrate.js#L41-L53. But on this branch it would also need to copy all the files in the scripts/offliner/ subdirectory.
  2. offliner is licensed under an Apache 2.0 license, but in release template files to public domain #38 we plan to release all template files to the public domain, so we would need to be able to release the offliner files in the templates/ directory to the public domain as well.

@delapuente
Copy link
Contributor Author

After talking with @mykmelez, we decided to close this issue without merging and I will be writing the service worker we need to address #154 and #5.

Apart from the legal issue of making all of public domain, which is a minor problem, the main reason is the functionality covered by offliner is beyond the scope of oghliner v1. Both offliner and oghliner are trying to provide an application lifecycle from different approaches and both need fine-grain control on service workers. My intention integrating offliner was to provide flexibility while avoiding rethink the application lifecycle but for oghliner v1 we want to experiment with a simpler solution more tied to the service worker lifecycle.

Thus, to regain full control on the lifecycle, we are writing our own service worker which will provide the precise functionality we want.

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

Successfully merging this pull request may close these issues.

None yet

3 participants