Refactor driver usage to improve Venia portability #1217

zetlen · 2019-05-07T19:22:34Z

This PR is a cherry-pick from #1198 to make that PR more digestible.

It is number 3 of 6 in the chain of cherry-picked PRs. It needs to be merged before the next 3:

Description

When trying to use Venia components in a context with another app, it's easy to get React errors claiming you're trying to use a Link element without a Router in context. This mostly happens because we forgot a couple of spots where we are hardcoding a particular instance of React Router instead of pulling in the instance provided by @magento/venia-drivers. This PR does the refactoring and prop drilling necessary to pass drivers around, so that an app can share routing with other components.

Related Issue

Closes #ISSUE_NUMBER.

Verification Steps

Ensure yarn build is successful
Run yarn watch:venia and load up your local Venia in your browser.
Ensure the main (CMS) page works.
Click through to the Create Account page, ensure that works.
Execute a search and ensure that works.
Hit a page that doesn't exist (ex: /noexist.html) and ensure you see the "Something went wrong. Please try again" message.
Sign In and ensure you can pull up your account by clicking on your user information:

How Have YOU Tested this?

Screenshots / Screen Captures (if appropriate):

Proposed Labels for Change Type/Package

major (e.g x.0.0 - a breaking change)
minor (e.g 0.x.0 - a backwards compatible addition)
patch (e.g 0.0.x - a bug fix)

Checklist:

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

…lish Fixes #1214.

vercel · 2019-05-07T19:22:40Z

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

Latest deployment for this branch: https://venia-git-zetlen-refactor-router-drivers-for-portability.magento-research1.now.sh

coveralls · 2019-05-07T19:29:48Z

Coverage remained the same at 78.195% when pulling d309335 on zetlen/refactor-router-drivers-for-portability into 8f64f51 on zetlen/remove-unused-buildpack-tools.

supernova-at · 2019-05-08T19:29:16Z

packages/peregrine/package.json

-    "@babel/plugin-transform-runtime": "~7.3.4",
-    "@babel/runtime": "~7.3.4",
+    "@babel/plugin-transform-runtime": "~7.4.4",
+    "@babel/runtime": "~7.4.2",


Since it's listed in peerDependencies do we need @babel/runtime here?

Yeah, for development.

@supernova-at Entries in peerDependencies aren't installed when you run yarn (or npm install). They're needed for tests, though, so we have to list them in devDependencies as well.

supernova-at · 2019-05-08T19:30:06Z

packages/venia-concept/package.json

@@ -47,14 +44,14 @@
    "apollo-link-http": "~1.5.11",
    "braintree-web-drop-in": "~1.16.0",
    "graphql-cli": "~3.0.11",
-    "graphql-cli-validate-magento-pwa-queries": "~1.0.0",
+    "graphql-cli-validate-magento-pwa-queries": "~1.1.0-beta.1",


Was this change for testing purposes or do we actually want to commit it?

For testing. Removing it.

supernova-at · 2019-05-08T19:34:26Z

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

-        <Route render={() => <Page>{renderRoutingError}</Page>} />
+        <Route
+            render={() => (
+                <Page using={{ Route, Router }}>{renderRoutingError}</Page>


More of a curiosity question: is it possible for the Route component being rendered here and the Route being passed in the using prop to Page to be different? Would you ever actually want to do that?

It's possible. And honestly it's easier to just drill the props through. <Page /> is its own component and could be used separately by something in the future, so rather than try to make a new ContextProvider and think about all those use cases, I thought props would be simpler code.

supernova-at · 2019-05-08T19:37:24Z

pwa-devdocs/src/peregrine/routing/index.md

@@ -144,6 +145,6 @@ class App extends Component {
 [`Page.js`]: https://github.com/magento-research/pwa-studio/blob/master/packages/peregrine/src/Page/Page.js
 [`react-router`]: https://github.com/ReactTraining/react-router
 [React Context]: https://reactjs.org/docs/context.html
-[ErrorView]:t://github.com/magento-research/pwa-studio/blob/master/packages/venia-concept/src/components/ErrorView/errorView.js 
+[ErrorView]: https://github.com/magento-research/pwa-studio/blob/master/packages/venia-concept/src/components/ErrorView/errorView.js


supernova-at

Assuming my comments don't need to be addressed, 👍

…ility

zetlen · 2019-05-09T13:54:33Z

@supernova-at Thanks for reminding me about the beta version I left in there. I removed it and answered your question about passing Route around. I'd love it if you took another look (at this one before my others if possible) :)

dpatil-magento · 2019-05-09T15:23:52Z

@zetlen Home page does not load https://venia-e2aw4xl3e.now.sh/ console - Uncaught TypeError: Cannot read property 'location' of undefined

supernova-at · 2019-05-09T15:41:44Z

@zetlen Home page does not load https://venia-e2aw4xl3e.now.sh/ console - Uncaught TypeError: Cannot read property 'location' of undefined

I successfully verified on my local machine. Not sure where the disconnect is there. location is likely React Router's location.

…nother app" This reverts commit d309335.

zetlen · 2019-05-09T21:08:13Z

@dpatil-magento I've refactored this to be much simpler. It appears to be working now in staging--please validate if you have the time!

@supernova-at @jimbo No more passing the Route and Router pair around. I figured out what was causing the realm mismatch: it was just bad versioning and withRouter.

dpatil-magento · 2019-05-09T21:20:45Z

@zetlen Thanks, testing this one now.

jimbo

Looks good to me. I've left a couple of comments.

jimbo · 2019-05-09T22:52:34Z

packages/venia-concept/src/drivers/index.js

@@ -1,5 +1,5 @@
 export { Query } from 'react-apollo';
-export { Link, Redirect, Route } from 'react-router-dom';
+export { Link, Redirect, Route, Switch, withRouter } from 'react-router-dom';


Consider export * here, maybe? This is such a major dependency—I think whitelisting only some of its exports qualifies as a leaky abstraction.

If a consumer of venia-concept wanted to import useRouter(), for example, they'd encounter the same "conflicting context" issues that this change is addressing.

I'd rather export these things one by one to assist in tree shaking.

Yeah, I ordinarily wouldn't recommend export * for that reason. I think this counts as a serious flaw in this approach, though. If we're not going to export *, then we should whitelist all of its exports. And we should consider doing the same for the other third-party libraries being exported here as well.

packages/pwa-buildpack/package.json

dpatil-magento

QA Pass.

…ub.com:magento-research/pwa-studio into zetlen/refactor-router-drivers-for-portability

zetlen added 2 commits May 7, 2019 11:54

fix(publishing): copy magento-compatibility.js during graphql-cli pub…

fe83772

…lish Fixes #1214.

chore: remove unsupported and abandoned buildpack tools

8f64f51

zetlen changed the base branch from develop to zetlen/remove-unused-buildpack-tools May 7, 2019 19:23

zetlen force-pushed the zetlen/refactor-router-drivers-for-portability branch from e6d1d43 to 917853b Compare May 7, 2019 19:27

vercel bot deployed to staging May 7, 2019 19:27 View deployment

refactor: improve driver portability for consuming venia in another app

d309335

zetlen force-pushed the zetlen/refactor-router-drivers-for-portability branch from 917853b to d309335 Compare May 7, 2019 20:06

vercel bot deployed to staging May 7, 2019 20:06 View deployment

This was referenced May 8, 2019

Unified system for environment-based configuration #1224

Merged

fix(publishing): copy magento-compatibility.js during graphql-cli pub… #1215

Merged

chore: remove unsupported and abandoned buildpack tools #1216

Merged

zetlen added the create-pwa-app-deps label May 8, 2019

zetlen mentioned this pull request May 8, 2019

Graphql playground autodetects queries #1219

Merged

5 tasks

supernova-at reviewed May 8, 2019

View reviewed changes

supernova-at previously approved these changes May 8, 2019

View reviewed changes

supernova-at self-assigned this May 8, 2019

zetlen changed the base branch from zetlen/remove-unused-buildpack-tools to develop May 8, 2019 22:22

Merge branch 'develop' into zetlen/refactor-router-drivers-for-portab…

4ef62b2

…ility

zetlen dismissed supernova-at’s stale review via 4ef62b2 May 9, 2019 05:05

vercel bot deployed to staging May 9, 2019 05:05 View deployment

vercel bot deployed to staging May 9, 2019 05:15 View deployment

supernova-at previously approved these changes May 9, 2019

View reviewed changes

dpatil-magento assigned dpatil-magento and unassigned supernova-at May 9, 2019

dpatil-magento assigned zetlen and unassigned dpatil-magento May 9, 2019

fixup rev gql plugin dep back

31c2d8b

zetlen dismissed supernova-at’s stale review via 31c2d8b May 9, 2019 16:57

zetlen force-pushed the zetlen/refactor-router-drivers-for-portability branch from 687be44 to 31c2d8b Compare May 9, 2019 16:57

vercel bot deployed to staging May 9, 2019 16:57 View deployment

zetlen added 2 commits May 9, 2019 13:22

Revert "refactor: improve driver portability for consuming venia in a…

1f6d6ec

…nother app" This reverts commit d309335.

fixup move withRouter to drivers

2b74323

vercel bot deployed to staging May 9, 2019 20:54 View deployment

zetlen requested a review from dpatil-magento May 9, 2019 21:08

jimbo reviewed May 9, 2019

View reviewed changes

dpatil-magento reviewed May 10, 2019

View reviewed changes

vercel bot deployed to staging May 10, 2019 16:40 View deployment

vercel bot temporarily deployed to staging May 10, 2019 19:14 Inactive

vercel bot deployed to staging May 10, 2019 19:16 View deployment

refactor: improve driver portability for consuming venia in another app

4284fa1

zetlen force-pushed the zetlen/refactor-router-drivers-for-portability branch from d18b3d4 to 4284fa1 Compare May 10, 2019 20:09

vercel bot deployed to staging May 10, 2019 20:09 View deployment

Merge branch 'zetlen/refactor-router-drivers-for-portability' of gith…

9dd1f3f

…ub.com:magento-research/pwa-studio into zetlen/refactor-router-drivers-for-portability

vercel bot deployed to staging May 10, 2019 20:15 View deployment

fixup css-loader to caret version

3662206

vercel bot deployed to staging May 10, 2019 21:06 View deployment

jimbo approved these changes May 10, 2019

View reviewed changes

zetlen merged commit 1e1fef1 into develop May 11, 2019

zetlen deleted the zetlen/refactor-router-drivers-for-portability branch July 25, 2019 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor driver usage to improve Venia portability #1217

Refactor driver usage to improve Venia portability #1217

zetlen commented May 7, 2019 •

edited by supernova-at

Loading

vercel bot commented May 7, 2019 •

edited

Loading

coveralls commented May 7, 2019 •

edited

Loading

supernova-at May 8, 2019

zetlen May 8, 2019

jimbo May 9, 2019

supernova-at May 8, 2019

zetlen May 10, 2019

supernova-at May 8, 2019

zetlen May 9, 2019

supernova-at May 8, 2019

supernova-at left a comment

zetlen commented May 9, 2019

dpatil-magento commented May 9, 2019

supernova-at commented May 9, 2019 •

edited

Loading

zetlen commented May 9, 2019

dpatil-magento commented May 9, 2019

jimbo left a comment

jimbo May 9, 2019

zetlen May 10, 2019

jimbo May 10, 2019

dpatil-magento left a comment

Refactor driver usage to improve Venia portability #1217

Refactor driver usage to improve Venia portability #1217

Conversation

zetlen commented May 7, 2019 • edited by supernova-at Loading

Description

Related Issue

Verification Steps

How Have YOU Tested this?

Screenshots / Screen Captures (if appropriate):

Proposed Labels for Change Type/Package

Checklist:

vercel bot commented May 7, 2019 • edited Loading

coveralls commented May 7, 2019 • edited Loading

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

Choose a reason for hiding this comment

supernova-at left a comment

Choose a reason for hiding this comment

zetlen commented May 9, 2019

dpatil-magento commented May 9, 2019

supernova-at commented May 9, 2019 • edited Loading

zetlen commented May 9, 2019

dpatil-magento commented May 9, 2019

jimbo 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

dpatil-magento left a comment

Choose a reason for hiding this comment

zetlen commented May 7, 2019 •

edited by supernova-at

Loading

vercel bot commented May 7, 2019 •

edited

Loading

coveralls commented May 7, 2019 •

edited

Loading

supernova-at commented May 9, 2019 •

edited

Loading