Supplant MagentoRouteHandler #1966

jimbo · 2019-11-08T02:37:34Z

Description

Now that we're using React Router 5.1.0, which includes hooks and real context, we no longer need our customized router. Not only can we replace custom code with canonical implementations (similar to our adoption of Apollo hooks), we can eliminate render props at the base of the app, which should improve rendering performance across the board.

Added

The new Routes component replaces renderRoutes()
The new MagentoRoute component replaces MagentoRouteHandler
The new useMagentoRoute talon replaces MagentoRouteHandler

Changed

VeniaAdapter now renders BrowserRouter directly
The Apollo client object now exposes apiBase

Related Issue

PWA-137

Acceptance

This is a one-for-one refactor; just ensure there no regressions.

Verification Stakeholders

@dpatil-magento

Verification Steps

Navigate to the home page
Navigate to a category page and product page
Navigate to a known nonexistent page (e.g., /foo.html)
Navigate to the search page

Checklist

I have not updated the documentation accordingly, if necessary.
I have not added tests to cover my changes, if necessary.

PWAStudioBot · 2019-11-08T02:41:33Z

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Associated JIRA tickets: PWA-137.

Generated by 🚫 dangerJS against 79b9b33

packages/peregrine/lib/talons/MagentoRoute/useMagentoRoute.js

revanth0212

I feel we should use the SW for caching purposes instead of manipulating cache ourselves. Along with that couple other minor requests/queries.

revanth0212 · 2019-11-08T20:31:51Z

packages/peregrine/lib/talons/MagentoRoute/addToCache.js

@@ -0,0 +1,15 @@
+const addToCache = async (...urls) => {
+    if (!window.caches) {


It is good to see caching but can we delegate this work to the SW? 2 reasons:

No main thread cycles wasted

cacheName can change in the future and having it hardcoded here can be a potential cause for bugs. We can leave this to the SW and if the cacheName changes in the future workbox will take care of it.

You can use sendMessageToSw to send a message along with the list of URL you want to cache and have a handler on the SW do the caching part.

For instance:

On the UI:

sendMessageToSW('PREFETCH_URLS', { urls: ['https://develop.pwa-venia.com/main.css', 'https://develop.pwa-venia.com/font.woff'] })

In SW:

registerMessageHandler('PREFETCH_URLS', prefetchUrls)

Check out this example:

pwa-studio/packages/peregrine/lib/talons/ProductImageCarousel/useProductImageCarousel.js

Line 36 in 21aadbc

sendMessageToSW(PREFETCH_IMAGES, {

pwa-studio/packages/venia-concept/src/ServiceWorker/Utilities/imageCacheHandler.js

Line 144 in 21aadbc

registerMessageHandler(PREFETCH_IMAGES, handleImagePreFetchRequest);

Good feedback, I agree with you on all points. I'd like to handle the change in a separate PR, though. This one is just a 1:1 refactor—nothing about addToCache has changed here.

The addToCache implementation is different here, though. The original implementation had some big problems, so I'm not about bringing it back, but I think this isn't populating the cache correctly. Can you confirm that it's actually caching and reusing the urlResolver responses? I think this is just caching an app shell instead of a GraphQL response.

Call me crazy, but did I do something other than just move this code into its own file?

packages/peregrine/lib/talons/MagentoRoute/getRouteComponent.js

packages/venia-ui/lib/components/Routes/routes.js

supernova-at

Awesome!

@zetlen made some changes recently so that we might recover from a temporary error state (and not cache it and return it forevermore).

Just making sure these changes don't regress that functionality.

packages/venia-ui/lib/components/Routes/routes.js

packages/venia-ui/lib/components/App/app.js

packages/venia-ui/lib/components/MagentoRoute/magentoRoute.js

packages/peregrine/lib/talons/MagentoRoute/useMagentoRoute.js

tjwiebell · 2019-11-11T19:31:39Z

packages/venia-ui/lib/RootComponents/Search/search.js

@@ -1 +0,0 @@
-export { default } from '../../components/SearchPage';


I thought this felt redundant 👍

zetlen

Extremely in favor of this. There are other optimizations that we ought to do for resolving unknown routes, but you're right that this PR's scope is just right as it is.

Not explicitly approving because I'd like to know if the addToCache implementation is actually working. Here's the repro for that.

Clear app cache, start a new (non-incognito) session.
Load homepage.
Click Dresses category
Back button to homepage
Soft-refresh page to wipe in-memory cache
Open network inspector, make sure cache is not disabled
Click Dresses category again

Expected: It reuses the cached route resolution and doesn't call /graphql again with urlResolver.

If the cache is working, that's great. If it isn't working, can we just comment it out for now? (The original cache had no TTL, so I'm glad it's gone.)

zetlen · 2019-11-12T01:05:33Z

packages/peregrine/lib/talons/MagentoRoute/addToCache.js

@@ -0,0 +1,15 @@
+const addToCache = async (...urls) => {
+    if (!window.caches) {


The addToCache implementation is different here, though. The original implementation had some big problems, so I'm not about bringing it back, but I think this isn't populating the cache correctly. Can you confirm that it's actually caching and reusing the urlResolver responses? I think this is just caching an app shell instead of a GraphQL response.

zetlen · 2019-11-12T01:06:20Z

packages/peregrine/lib/talons/MagentoRoute/getRouteComponent.js

+const getRouteComponent = async (apiBase, pathname) => {
+    // At build time, `fetchRootComponent` is injected as a global.
+    // Depending on the environment, this global will be either an
+    // ES module with a `default` property, or a plain CJS module.


Very clear, thank you.

zetlen · 2019-11-12T01:06:40Z

packages/peregrine/lib/talons/MagentoRoute/getRouteComponent.js

+    } catch (e) {
+        const routeError = e.message === '404' ? NOT_FOUND : INTERNAL_ERROR;
+
+        console.error(e);


Thank you for this, too.

sirugh · 2019-11-12T15:32:01Z

Expected: It reuses the cached route resolution and doesn't call /graphql again with urlResolver.

I'm getting the expected outcome. Can someone else check this as well?

sirugh

A few comments, nothing hold this PR up. Great work!

Also, FWIW I tried to repro the issue with addToCache that @zetlen mentioned but I could not. Hopefully someone else can double check!

sirugh · 2019-11-12T15:35:02Z

packages/venia-ui/lib/drivers/adapter.js

        return client;
    }, [apiBase, apollo]);

    return (
        <ApolloProvider client={apolloClient}>
            <ReduxProvider store={store}>
-                <Router apiBase={apiBase}>{children}</Router>


Just to clarify we are intentionally not removing the unused Router implementation right? We also have some old devdocs for it so I want to make sure we're not leaving outdated references lying around.

Correct. I didn't delete any of the old implementations since it wasn't strictly necessary to achieve the goal here. So we have a choice:

Remove unused files, such as Router and MagentoRouteHandler

Deprecate unused files, but remove them later on

sirugh · 2019-11-12T15:38:52Z

packages/venia-ui/lib/components/App/app.js

@@ -9,7 +9,7 @@ import Main from '../Main';
 import Mask from '../Mask';
 import MiniCart from '../MiniCart';
 import Navigation from '../Navigation';
-import renderRoutes from './renderRoutes';


Same question here -- we're not deleting the old stuff on purpose, right?

sirugh · 2019-11-12T15:55:50Z

packages/peregrine/lib/talons/MagentoRoute/useMagentoRoute.js

+                    }
+
+                    // add the pathname to the browser cache
+                    addToCache(pathname);


In the old implementation we had a catch that logged errors in development. Can we add that here? I know we're probably changing this soon so nbd either way.

I believe that is here still. One of the other reviewers suggested that I remove the process.env check since it was redundant, so I did that.

Cool, didn't realize we eliminated consoles in prod.

Supplant MagentoRouteHandler with Routes

fec971c

jimbo requested review from zetlen, sirugh and revanth0212 November 8, 2019 02:38

PWAStudioBot added pkg:peregrine pkg:venia-ui labels Nov 8, 2019

jimbo commented Nov 8, 2019

View reviewed changes

packages/peregrine/lib/talons/MagentoRoute/useMagentoRoute.js Outdated Show resolved Hide resolved

packages/peregrine/lib/talons/MagentoRoute/useMagentoRoute.js Show resolved Hide resolved

m2-community-project bot assigned jimbo Nov 8, 2019

m2-community-project bot added the Progress: review label Nov 8, 2019

revanth0212 requested changes Nov 8, 2019

View reviewed changes

m2-community-project bot assigned revanth0212 Nov 8, 2019

m2-community-project bot added Progress: changes requested and removed Progress: review labels Nov 8, 2019

supernova-at reviewed Nov 11, 2019

View reviewed changes

m2-community-project bot assigned supernova-at Nov 11, 2019

m2-community-project bot added Progress: review and removed Progress: changes requested labels Nov 11, 2019

jimbo added 3 commits November 11, 2019 10:48

Handle PR feedback

7a7ef14

Address PR feedback

5882424

Export error strings

701f7bf

supernova-at reviewed Nov 11, 2019

View reviewed changes

packages/peregrine/lib/talons/MagentoRoute/useMagentoRoute.js Outdated Show resolved Hide resolved

Fix unit tests

c9b9031

supernova-at mentioned this pull request Nov 11, 2019

Bugfix/fixing storybook venia ui #1945

Merged

tjwiebell reviewed Nov 11, 2019

View reviewed changes

m2-community-project bot assigned tjwiebell Nov 11, 2019

Remove client-only route consideration

cabd8ec

supernova-at approved these changes Nov 11, 2019

View reviewed changes

Merge branch 'develop' into jimbo/magento-routes

9beca88

zetlen reviewed Nov 12, 2019

View reviewed changes

m2-community-project bot assigned zetlen Nov 12, 2019

sirugh reviewed Nov 12, 2019

View reviewed changes

m2-community-project bot assigned sirugh Nov 12, 2019

Merge branch 'develop' into jimbo/magento-routes

e1c7551

revanth0212 approved these changes Nov 12, 2019

View reviewed changes

sirugh mentioned this pull request Nov 13, 2019

[bug]: Uncaught Invariant Violation: You must pass a component to the function returned by connect. Instead received undefined #1590

Closed

7 tasks

Merge branch 'develop' into jimbo/magento-routes

79b9b33

sirugh mentioned this pull request Nov 13, 2019

[bugfix]: Fixes hot reloading #1972

Merged

dpatil-magento merged commit d0164ba into develop Nov 13, 2019

dpatil-magento deleted the jimbo/magento-routes branch November 13, 2019 21:52

m2-community-project bot removed the Progress: review label Nov 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supplant MagentoRouteHandler #1966

Supplant MagentoRouteHandler #1966

jimbo commented Nov 8, 2019 •

edited by sirugh

Loading

PWAStudioBot commented Nov 8, 2019 •

edited

Loading

revanth0212 left a comment

revanth0212 Nov 8, 2019

jimbo Nov 11, 2019

zetlen Nov 12, 2019

jimbo Nov 12, 2019

supernova-at left a comment

tjwiebell Nov 11, 2019

zetlen left a comment

zetlen Nov 12, 2019

zetlen Nov 12, 2019

zetlen Nov 12, 2019

sirugh commented Nov 12, 2019

sirugh left a comment

sirugh Nov 12, 2019

jimbo Nov 12, 2019

sirugh Nov 12, 2019

sirugh Nov 12, 2019

jimbo Nov 12, 2019

sirugh Nov 12, 2019

		@@ -0,0 +1,15 @@
		const addToCache = async (...urls) => {
		if (!window.caches) {

		@@ -1 +0,0 @@
		export { default } from '../../components/SearchPage';

Supplant MagentoRouteHandler #1966

Supplant MagentoRouteHandler #1966

Conversation

jimbo commented Nov 8, 2019 • edited by sirugh Loading

Description

Related Issue

Acceptance

Verification Stakeholders

Verification Steps

Checklist

PWAStudioBot commented Nov 8, 2019 • edited Loading

revanth0212 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supernova-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetlen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirugh commented Nov 12, 2019

sirugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimbo commented Nov 8, 2019 •

edited by sirugh

Loading

PWAStudioBot commented Nov 8, 2019 •

edited

Loading