Closed samuelmeuli closed 4 years ago
@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?
@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 https://github.com/facebook/create-react-app/pull/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 #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:
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.
[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.
Forking CRA and editing the Webpack config. Will lead to higher maintenance effort, but might be the cleanest solution for users.
Ejecting CRA and editing the Webpack config. That way, we would miss out on future features/fixes of CRA.
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
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...
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.
react-app
andangular-app
folders.react-scripts-rewired
, code splitting is now possible and configured to work by default with React (see the docs).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).