feat(devex): Refactor Venia to be portable and importable #788

zetlen · 2019-01-28T21:13:35Z

Description

Change the build configuration and dependency graph of Venia and Peregrine so that community developers can use individual Venia components in external apps.

Add "partially transpiled" build targets to Venia and Peregrine, producing ES Modules that external build processes can parse, and that web packaging tools can tree-shake.
- Add and refactor package scripts and Babel configuration
- Add module resolver plugin to Babel so it transpiles package-relative paths
- Add export directives to Peregrine package.json and Venia package.json to tell dependency managers where to find different transpilations and/or raw source
Refactor Venia components to pull their context-driven dependencies from a single aggregate file of such deps, "drivers"
- Reroute all imports of <Query />, connect(), <Link >, <Route />, and others through the central drivers file
- Consolidate Context Providers into an also-exported <VeniaAdapter />
- Adjust babel config to map the drivers file to a friendly, unique dep name @magento/venia-drivers
Refactor Webpack plugins and other build time configuration to streamline builds and reduce custom code
- Add doc comments

Motivation and Context

PWA Studio needs to be modular and its components need to be usable and mix-and-matchable at release time. We don't offer "inheritance" in the way of Magento themes, so we have to live up to our promise of modularity and composability. We sacrificed some of that for velocity, and it's time to get that back.

How Has This Been Tested?

Refactored unit tests and restored coverage.
Tested all changed code.
Ran all NPM scripts.

Screenshots (if appropriate):

I built an example app to show how these changes enable import of Venia components á la carte:

https://venia-consuming-app.now.sh/

Its source code is at https://github.com/magento-research/venia-consumer-example and a build artifact is inspectable at https://venia-consuming-app.now.sh/_src . Check it out!

Proposed Labels for Change Type/Package

feature, documentation

peregrine, pwa-buildpack, venia-concept

Checklist:

I have read the CONTRIBUTING document.
I have linked an issue to this PR.
I have indicated the change type and relevant package(s).
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
All CI checks are green (linting, build/deploy, etc).
At least one core contributor has approved this PR.

vercel · 2019-01-28T21:13:41Z

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

jimbo

Nice. This is a clean set of changes; it's mostly just import swaps, as we discussed. I've left comments in a few places. Other things we've got to do:

Workshop the name of @magento-venia/drivers. It feels inconsistent with our other packages.
Remove package-lock.json. Looks like at least one is included in this diff.

jimbo · 2019-01-29T00:05:17Z

packages/venia-concept/src/components/MiniCart/product.js

-            backgroundImage: `url(${makeProductMediaPath(image.file)})`
+            backgroundImage: `url(${new ResourceUrl(
+                makeProductMediaPath(image.file)
+            ).toString()})`


We could omit .toString() in places like this, right? It should be automatically called for expressions in template literal placeholders.

You're right that this would be called implicitly, but that's a little bit magic. It's also there to indicate that ResourceUrl will probably have other instance methods in the future.

Naming that method toString is the magic part, though. 😉

packages/pwa-buildpack/.babelrc.js

packages/peregrine/src/Router/MagentoRouteHandler.js

jimbo · 2019-01-29T00:28:10Z

package.json

    "bundlesize": "~0.17.1",
    "chalk": "~2.4.2",
    "chokidar": "~2.0.4",
+    "concurrently": "~4.1.0",


Is this being used anywhere?

Yeah, in Venia's build script.

Looks like it's used in the Peregrine's build script, not Venia's. In either case, the dependency should be listed as a devDependency of the package itself, not here in the root.

yarn workspace @magento/peregrine add --dev --tilde concurrently

@jimbo Thanks. You shouldn't need the tilde anymore now that we've got a .yarnrc.

Speaking of which: One downside of Yarn is that it doesn't warn you on the command line if two packages have slightly different semver strings for the same dependency. I think we should strive to not let our dependencies between packages drift apart.

jimbo · 2019-01-29T00:29:52Z

package.json

@@ -45,9 +46,11 @@
  "devDependencies": {
    "@magento/eslint-config": "~1.3.0",
    "babel-eslint": "~10.0.1",
+    "babel-plugin-module-resolver": "^3.1.3",


Consider --tilde here.

Whoa, how'd that get in there?

Looks like Yarn doesn't respect our .npmrc, so I added a .yarnrc to set the default save prefix.

zetlen · 2019-01-29T02:14:55Z

@jimbo Thanks for taking a look. What would you suggest instead of @magento-venia/drivers?

jochienabuurs · 2019-01-29T14:27:30Z

@zetlen would the same approach be viable for https://github.com/magento-research/pwa-studio/tree/release/2.0/packages/peregrine

zetlen · 2019-01-29T15:42:44Z

@jochienabuurs It would! The only reason I did this for Venia first is that Peregrine is about to go through some major API changes after our release. For now, large Peregrine changes are on hold, and Venia is the more useful and feature-complete of the two.

However, this PR also makes the existing Peregrine components easier to import.

zetlen · 2019-01-29T16:52:28Z

Switched to @magento/venia-drivers based on @jimbo 's observation that @magento/ is our consistent NPM namespace. Good catch! Any other @magento-research/core-pwa-devs have input? I'd like to expedite this

sirugh

This is great! I couldn't find anything that jumped out to me, and it all makes sense. Do we have a follow-up issue for @jcalcaben to add/update docs?

And 👍 for the rename to @magento/venia-drivers.

packages/venia-concept/package.json

jimbo · 2019-01-29T17:59:00Z

@zetlen Thanks for responding to the feedback. Take a look through the comments again. One or two minor changes remain, then we're good.

jimbo · 2019-01-29T21:55:15Z

@zetlen This is approved and ready to go.

I'd like to note here that we should probably find a way to do something about the noisy warnings about module resolution. Same for the missing peer dependencies from my Yarn PR. Let's figure out what our options are.

feat(devex): Refactor Venia to be portable and importable

93436f3

zetlen requested review from sirugh, jimbo, pcvonz and supernova-at January 28, 2019 21:13

cherdman added the 2.0 release label Jan 28, 2019

cherdman requested review from tjwiebell and jcalcaben January 28, 2019 22:03

jimbo suggested changes Jan 29, 2019

View reviewed changes

fixup npmrc and stray package-lock

e20dd94

Merge branch 'release/2.0' into zetlen/portable-venia-components

d4e3a9a

zetlen force-pushed the zetlen/portable-venia-components branch from 080f285 to d4e3a9a Compare January 29, 2019 02:16

sirugh previously approved these changes Jan 29, 2019

View reviewed changes

sirugh reviewed Jan 29, 2019

View reviewed changes

packages/venia-concept/package.json Outdated Show resolved Hide resolved

fixup switch @magento-venia/drivers to @magento/venia-drivers

7aa4563

zetlen dismissed sirugh’s stale review via 7aa4563 January 29, 2019 17:04

zetlen force-pushed the zetlen/portable-venia-components branch from 1560ede to 7aa4563 Compare January 29, 2019 17:04

zetlen added 3 commits January 29, 2019 12:28

fixup move concurrently devdep

acda98b

fixup: replace ResourceUrl class with resourceUrl function

130fb7f

refactor: merge image changes and add resize to resourceUrl

b5de6c7

zetlen force-pushed the zetlen/portable-venia-components branch from 77aa493 to b5de6c7 Compare January 29, 2019 19:52

jimbo approved these changes Jan 29, 2019

View reviewed changes

Merge branch 'release/2.0' into zetlen/portable-venia-components

a40c4a7

jimbo merged commit 441b648 into release/2.0 Jan 29, 2019

jimbo deleted the zetlen/portable-venia-components branch January 29, 2019 21:56

This was referenced Jan 30, 2019

[Documentation] Venia component portability #796

Closed

[Documentation] Venia modular components feature #800

Merged

marcin-piekarski mentioned this pull request Feb 8, 2019

"[BUG]" Venia Storybook Crash #862

Closed

6 tasks

zetlen mentioned this pull request Feb 10, 2019

[DEVEX] Build process logs multiple console warnings when building Venia ES Modules #880

Closed

6 tasks

zetlen mentioned this pull request Jun 28, 2019

[feature]: Shareable Babel "preset" package to simplify build config #1403

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(devex): Refactor Venia to be portable and importable #788

feat(devex): Refactor Venia to be portable and importable #788

zetlen commented Jan 28, 2019 •

edited

Loading

vercel bot commented Jan 28, 2019

jimbo left a comment •

edited

Loading

jimbo Jan 29, 2019

zetlen Jan 29, 2019

jimbo Jan 29, 2019

jimbo Jan 29, 2019

zetlen Jan 29, 2019

jimbo Jan 29, 2019

zetlen Jan 29, 2019

jimbo Jan 29, 2019

zetlen Jan 29, 2019

zetlen Jan 29, 2019

zetlen commented Jan 29, 2019

jochienabuurs commented Jan 29, 2019

zetlen commented Jan 29, 2019

zetlen commented Jan 29, 2019

sirugh left a comment

jimbo commented Jan 29, 2019

jimbo commented Jan 29, 2019 •

edited

Loading

feat(devex): Refactor Venia to be portable and importable #788

feat(devex): Refactor Venia to be portable and importable #788

Conversation

zetlen commented Jan 28, 2019 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Proposed Labels for Change Type/Package

Checklist:

vercel bot commented Jan 28, 2019

jimbo left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetlen commented Jan 29, 2019

jochienabuurs commented Jan 29, 2019

zetlen commented Jan 29, 2019

zetlen commented Jan 29, 2019

sirugh left a comment

Choose a reason for hiding this comment

jimbo commented Jan 29, 2019

jimbo commented Jan 29, 2019 • edited Loading

zetlen commented Jan 28, 2019 •

edited

Loading

jimbo left a comment •

edited

Loading

jimbo commented Jan 29, 2019 •

edited

Loading