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

Offline mode and web app manifest #486

Merged
merged 23 commits into from
Nov 28, 2018
Merged

Offline mode and web app manifest #486

merged 23 commits into from
Nov 28, 2018

Conversation

pcvonz
Copy link
Contributor

@pcvonz pcvonz commented Nov 9, 2018

This PR is a:

[x] New feature
[ ] Enhancement/Optimization
[ ] Refactor
[ ] Bugfix
[ ] Test for existing code
[ ] Documentation

Summary

Adds the beginnings of offline mode to Venia

When this pull request is merged, it will...

  • Cache every visited route as an HTML file via Workbox. This will enable the ability to navigate directly to a page when offline.
  • Cache all RootComponents and a map of every visited URL and the response from urlResolver. This allows loading RootComponents when offline.
  • Add a new RootComponent, NotFound, which is loaded when offline and the requested page isn't cached.
  • Add an offline indicator, displays when offline and when transitioning from offline to online.
  • Add the option to use InjectManifest in the ServiceWorkerPlugin.
  • Add a web-app-manifest to the project so Venia can be installed like a native app.
  • Add apollo-cache-persist which caches graphql queries in localStorage so they can be made offline.
  • Add copy-webpack-plugin to Venia so assets (mainly for the manifest) can be moved into the dist directory and be served by upward.

Additional information

Relevant issues:

Future considerations

  • It'd be nice to add the ability to define the name of the runtime cache.
  • the NotFound root component is hard-coded into resolveUnknownRoute. Will something that can serve this purpose be added to the Magento back-end?
  • The online / offline indicator could be made into a generic toast.
  • Caching the response from urlResolver will be a lot easier once the graphql back-end supports GET requests.

online-offline

pcvonz and others added 16 commits October 19, 2018 10:17
- Add troubleshooting section for `connection not secure`
- Add Apollo simple persistence for persistent cache of graphql queries
- Add html files to SW runtime cache
- Fix venia favicon (fixes serving images via upward in general)
- Add new venia icons
- If offline loads component from localStorage
- If offline and component not in cache, load NotFound root component
- Add fallback page to venia-upward yml
- Switch to inject-manifest in webpack
- Add test for offline functionality of resolveUnkownRoute
- Add test for injectManifest of ServiceWorkerPlugin
- SW.js is now served with `Service-Worker-Allowed` header
- manifest icons are moved to the dist directory with
`copy-webpack-plugin`
- manifest.webmanifest -> manifest.json
- `getRouteComponent` now checks if the current route has the
`NotFoundComponent` stored in state. If so, reload the route when
online

Progress towards #374
- Shows online status when switching from offline -> online
- Animation for both online and offline
- ServiceWorkerPlugin takes in `injectManifestConfig`
- Update tests
- Cache chunks via workbox-webpack
@magento-cicd2
Copy link

magento-cicd2 commented Nov 9, 2018

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented Nov 9, 2018

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@pcvonz pcvonz requested review from zetlen and jimbo November 9, 2018 18:37
@coveralls
Copy link

coveralls commented Nov 9, 2018

Coverage Status

Coverage decreased (-0.4%) to 16.98% when pulling e99a80a on bargreenellingson:offline-mode-web-app-manifest into 768b4ca on magento-research:release/2.0.

@rowan-m
Copy link
Contributor

rowan-m commented Nov 9, 2018

/packages/venia-concept/templates/doctype-and-head-start.mst

  • Add <meta name="theme-color" content="#ff6334">

@rowan-m
Copy link
Contributor

rowan-m commented Nov 9, 2018

Styling question regarding the online / offline indicator. Can this overlay rather than push content down? I'm concerned this could lead to some janky painting as it appears and disappears.

@pcvonz
Copy link
Contributor Author

pcvonz commented Nov 9, 2018

Styling question regarding the online / offline indicator. Can this overlay rather than push content down? I'm concerned this could lead to some janky painting as it appears and disappears.

The offline indicator is persistent. I didn't want to obscure any of the content so I let it push the content down. We could perhaps allow the indicator to be dismissed.
Maybe @soumya-ashok could provide some input.

Copy link
Contributor

@jimbo jimbo left a comment

Choose a reason for hiding this comment

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

@pcvonz This is looking good. I have a few changes to request while I test it out locally. We'll try to get this merged pretty soon here, though.

});
} else {
return new Promise(function(resolve) {
resolve({
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to use Promise.resolve() here. Alternately, you could make remotelyResolveRoute an async function and just return things directly.

let urlResolve = localStorage.getItem('urlResolve');
urlResolve = JSON.parse(urlResolve);
urlResolve = urlResolve ? urlResolve : {};
urlResolve[opts.route] = res;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider declaring more than one variable here instead of reusing the same one, so it's easier to read.

const itemString = localStorage.getItem('urlResolve')
const item = JSON.parse(storedRoute) || {}

item[opts.route] = res
localStorage.setItem('urlResolve', JSON.stringify(item))

};
};

export default connect(
Copy link
Contributor

Choose a reason for hiding this comment

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

When merge conflicts are resolved and these changes ported to app.js, make sure to migrate the redux connections to App/container.js.

@@ -5,18 +5,8 @@
z-index: 1;
}

.page {
min-height: 100vh;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding it back since we're overlaying the notification.
If you were curious though:
100vh

100% {
height: 0;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

As Rowan had mentioned, animating height will cause render jank. I second his recommendationt to make it an overlay.

In addition, height is not a GPU-accelerated property, so we should avoid animating it. Try to restrict your animations to transform and opacity.

@@ -7,7 +7,8 @@ export const name = 'app';
const initialState = {
drawer: null,
overlay: false,
pending: {}
pending: {},
isOnline: navigator.onLine
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw earlier that the App component was keeping local state for whether it had ever been online. Why would we not put that here in redux state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll look into adding it.

Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

@pcvonz Thanks for your patience. This is awesome, exciting work that @rowan-m should be happy to see. I just pushed a manual back-merge from release/2.0 with a small change I should explain:

I unified our strategy for serving files that aren't Webpack output assets. This PR, #534, and some earlier work I did all had different approaches to this; I would prefer to use UPWARD to statically serve an additional directory, rather than use a CopyWebpackPlugin, so I moved things around. There is now a static directory for serving all files that are production-ready assets and don't need to go through Webpack. venia-upward.yml defines two DirectoryResolvers for static files, static and dist. Right now, there's a manually maintained regex in venia-upward for targeting the static resolver instead of dist. Any more files we add to static, we need to keep that regex up to date. But at least it's in one place!

This strategy will probably change in the future, since we want to integrate the build more and more, but for now, it's the simplest option. If we let multiple strategies for this sit around in the codebase, it'll grow way too confusing.

Besides that and a few other small changes from @jimbo and myself, this is ready to go, which is awesome.

workbox.strategies.staleWhileRevalidate()
);

workbox.routing.registerRoute(
Copy link
Contributor

Choose a reason for hiding this comment

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

This works for static files, but catalog images should probably use staleWhileRevalidate. It would be this route, prepended with /media/catalog.

@PWAStudioBot
Copy link
Contributor

Fails
🚫

The following file(s) were not formatted with prettier. Make sure to execute npm run prettier locally prior to committing.

packages/peregrine/src/Router/resolveUnknownRoute.js
packages/venia-concept/src/sw.js
🚫

The following file(s) did not pass ESLint. Execute npm run lint locally for more details

packages/venia-concept/src/components/App/app.js

Generated by 🚫 dangerJS

@jimbo jimbo merged commit 2c65cfc into magento:release/2.0 Nov 28, 2018
@jimbo
Copy link
Contributor

jimbo commented Nov 28, 2018

@pcvonz Merged! Thanks for taking this feature on! 🎉

@rowan-m
Copy link
Contributor

rowan-m commented Nov 29, 2018

Thank you @pcvonz this is a strong foundation!

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.

7 participants