Skip to content
This repository has been archived by the owner on Feb 19, 2020. It is now read-only.

Add PWA features #98

Closed
wants to merge 30 commits into from
Closed

Add PWA features #98

wants to merge 30 commits into from

Conversation

samuelmeuli
Copy link
Contributor

@samuelmeuli samuelmeuli commented Aug 13, 2019

Q                       A
Fixed Issues? Closes #61, closes #83
Patch: Bug Fix? No
Minor: New Feature? No
Major: Breaking Change? Yes
Tests Added + Pass? Yes
Documentation Provided Yes
Any Dependency Changes? Yes
License Apache License, Version 2.0

This PR configures the archetype to generate projects with features of a Progressive Web App.

The features can be tested using Lighthouse. All checks should pass with this setup.

  • A detailed description of the PWA setup and features can be found in the docs directory and the README files of the react-app and angular-app folders.
  • With the usage of react-scripts-rewired, code splitting is now possible and configured to work by default with React (see the docs).
  • The README files have been extended and improved (links fixed, steps clarified and duplicate parts removed).
  • The Angular dev server now uses port 3000 by default (for consistency with the React app).
  • The browser is no longer opened when starting the React dev server (URL was incorrect).
  • For the Angular app, environment variables are used instead of the environment.ts file (for consistency with the React app and to allow the usage of the variables in the Webpack configuration and service worker template).

Note: Currently, all files are served by AEM with a Service-Worker-Allowed header. This will be improved in a future PR using a Servlet Filter (so only service worker files are served with that header).

It is not a component, so it should not be in the `components` directory
CRA opens localhost:3000 at root path, which is not where the app is served
Instead of ejecting, the `config-overrides.js` file should be used to configure CRA as desired
Allows for simpler setup ("development" with `npm start` and "production" with `npm run build`, simplifies env variables), consistent with React app
Allows multiple, environment-dependent env files. Makes the behavior of the Angular and React apps more consistent
* Add docs on PWA and code splitting
* Fix broken links
* Remove duplicate content
* Put all archetype docs into root README, project-specific docs into `archetype-resources` dir, frameworks-specific docs into `angular-app`/`react-app` dirs
Should only be used in demo
@samuelmeuli samuelmeuli marked this pull request as ready for review August 28, 2019 16:22
@godanny86
Copy link
Collaborator

@samuelmeuli , @pfauchere after a little digging I feel strongly that we should not bootstrap projects with the react-app-rewired. Looking at the Readme for the react-app-rewired it appears there is limited support and it falls outside of "official" recommendations by React/facebook. This, of course, is fine for many customers but they need to understand the risks ahead of time. Since all SPA editor projects should start from the archetype this seems like a dangerous path to send customers down by default.

I'd prefer to stick as close to: https://create-react-app.dev/docs/making-a-progressive-web-app as possible. Forgive me @samuelmeuli, remind me again what functionality the rewired-app provides that we needed? Maybe we can solve some of this at the Dispatcher/Apache level for things like loading the Service Worker at the root of the site? If there are some features of PWA that require the app to either be ejected and/or use react-app-rewired then maybe we can document these steps instead of forcing customers to not use CRA?

@samuelmeuli
Copy link
Contributor Author

samuelmeuli commented Aug 29, 2019

@godanny86 I agree that using react-scripts-rewired is not ideal, but in my opinion it's one of the cleaner approaches.

The main problems that need to be solved are the following:

  • The service worker generated by create-react-app is not enough for AEM. It only caches assets in the scope of the React app, i.e. no AEM resources from other locations. Therefore, we cannot achieve complete offline functionality with it. It looks like CRA will get more options for Workbox in the future (see Added options to allow for overrides to workbox-webpack-plugin facebook/create-react-app#5369), but since CRA uses the GenerateSW plugin and not InjectManifest, which is required for writing custom caching rules (see https://developers.google.com/web/tools/workbox/modules/workbox-webpack-plugin), this will not be enough either. Consequently, we need to generate our own custom service worker. I suggest using Workbox for this as well.

  • Code splitting can currently not be achieved. To get it to work, the entry chunk could be identified and used as the ClientLib, and the other chunks could be loaded dynamically. However, this identification of the entry file is currently not possible (see React Code splitting #83 for details). Modifying the CRA Webpack config allows us to fix this.

  • Multiple values that might be changed by users after generating the project (e.g. the app root path) are currently hardcoded in various places in the code. Having these as environment variables would be cleaner and would require users to only change them in one place. By generating e.g. the PWA manifest using Webpack, environment variables could be used in such files as well.

Possible solutions we've identified:

  1. Generating a custom service worker with Workbox after building the app using CRA and removing the undesired generated files. Environment variables could be replaced with a script which runs after the build. This approach isn't very clean or easy to understand, but avoids modifications to the CRA Webpack config. I initially implemented this and can send you the code if you like.

  2. [Current approach] Using react-scripts-rewired and customize-cra to edit CRA's Webpack configuration. This avoids any additional build steps and is easy to understand, but – as you mentioned – might cause maintainability issues.

  3. Forking CRA and editing the Webpack config. Will lead to higher maintenance effort, but might be the cleanest solution for users.

  4. Ejecting CRA and editing the Webpack config. That way, we would miss out on future features/fixes of CRA.

  5. Dropping CRA and using a custom Webpack config. Also easy to understand for users, but this requires them to extend the Webpack config if they need additional features (which might otherwise be part of CRA).

The situation for Angular is pretty much the same. The service worker and manifest files generated using angular-cli are not sufficient for our use case, and @angular-builders/custom-webpack also allows us to modify the default Webpack configuration. Therefore, most of the custom Webpack config from React can be applied to the Angular app as well.

My preferred approach would be 3, and possibly 2 and 5. @godanny86, please let me know what you think or if you have any other ideas :)

cc @pfauchere

@godanny86
Copy link
Collaborator

hi @samuelmeuli, thank you for the detailed response! The complexity of this problem is not lost on me and you have explored many different solutions. My point is that the archetype should be a starting point to accelerate a customer project but not at the cost of choosing an implementation path. To your point, there seems to be several approaches for implementing a PWA and without a definitive consensus, I think we should let projects choose the most appropriate path based on their app requirements.

My approach would be to continue to provide as basic of a project as possible and add as many PWA features that we can. If some advanced configurations like complete off-line functionality, advanced caching, and code-splitting cannot be achieved then so be it. We can still provide samples and recommendations on how to achieve these advanced configurations and let projects decide how to implement them. At least we are moving projects closer to PWA and in the future, if it turns out most customers want/need those advanced configurations we can make a determination then...

* Use fork of `create-react-app` to get custom SW and code splitting without having to use `react-scripts-rewired`
* Use `open-cli` to launch web app on `npm start` (avoids confusion because web app is served on a subpath and errors are logged when visiting the root path)
* Update service worker template for the latest Workbox beta
* Specify manifest and favicons by hand instead of using a Webpack plugin for consistency with CRA
* Rename env var back to `PUBLIC_URL` for consistency with CRA
* Rename React root div for consistency with CRA
* Add checks that required env vars are defined
* Remove unused dependencies
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.

React Code splitting [React] Support static CRA assets
2 participants