V4P2: feat(buildpack): Add Webpack configurator facade #1498

zetlen · 2019-07-31T14:04:50Z

Description

Pull complex Webpack configuration out of the Venia app and put it in an opinionated Buildpack utility.

This replaces most of the large webpack.config.js file in Venia. The entire PWA Studio runtime depends on the logic in that file; Peregrine, Buildpack, and UpwardJS make assumptions based on that file's configuration, which is an antipattern.

This PR adds a configureWebpack() function which returns a Promise for a Webpack configuration object. It takes parameters for a subset of configuration options, including:

context: Absolute path of the project root to ensure consistent behavior no matter the working directory; if webpack.config.js is in project root then this should be simply __dirname.
vendor: Names of modules to bundle into a commons bundle. Set to a sensible default list in Venia.
special: Map of modules to flag as containing 'special' functionality, to be treated differently by the Webpack plugin and loader chain. Keys should be module names. Values should be one or more of the following flags:
- esModules: Denotes that JS code for this module must adhere to the babel-preset-peregrine Babel preset. Code will be imported as modules using the same loader chain as local application code, enabling tree-shaking on node_modules dependencies.
- cssModules: Denotes that CSS code for this module must assign all CSS imports to a variable, to receive an object of dynamically generated CSS classes. Style will be imported using the same loader chain as local application stylesheets, enabling total CSS modularity.
- graphQLQueries: Denotes that JS code for this module contains GraphQL queries embedded in calls to the graphql-tag template literal. GraphQL will be preprocessed using the same loader chain as local application code.
- rootComponents: Denotes that this module includes a src/RootComponents or lib/RootComponents directory containing RootComponents to be processed by the RootComponentsPlugin. These RootComponents will be added to the map of all RootComponent mappings available to the application.
- upward: Denotes that this module includes an upward.yml file at root declaring some subset of server-side behavior it requires and/or supplies. This file will be merged, along with the upward.yml for any other modules you configure with this flag. It will merge down to a final upward.yml file produced as a build asset.

Also adds a lot of debuggability to Buildpack, and cleans up some of its APIs.

Closes #1502 along with #1497 and #1499.

ℹ️ Docs and tests TODO.

Verification Steps

In a fresh repo, run yarn run build.
Examine packages/venia-concept/webpack.config.js.
Try adjusting the configuration flags to see what happens.

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.

vercel · 2019-07-31T14:04:54Z

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://pwa-studio-git-zetlen-v4apip2-webpack-facade.mmansoor.now.sh

PWAStudioBot · 2019-07-31T14:10:24Z

	Warnings
⚠️	Found the word "TODO" in the PR description. Just letting you know incase you forgot :)

	Messages
📖	We are currently working on automating the PR metadata checks. Until that time, you may see failures related to labels/description/linked issues/etc even if you have fixed the problem. Failures will persist until the next push (assuming they are fixed).

Generated by 🚫 dangerJS against f6f3909

coveralls · 2019-07-31T14:12:37Z

Coverage decreased (-2.5%) to 77.232% when pulling 984ef97 on zetlen/v4api/p2-webpack-facade into e629de4 on zetlen/v4api/p1-use-relative-imports.

supernova-at · 2019-07-31T19:04:41Z

packages/pwa-buildpack/lib/WebpackTools/__tests__/PWADevServer.spec.js

@@ -7,6 +7,8 @@ jest.mock('../../Utilities/configureHost');
 const fs = require('fs');
 jest.spyOn(fs, 'readFile');

+const mockContext = require('path').resolve(__dirname, '../../../../../');
+
 const debugErrorMiddleware = require('debug-error-middleware');
 const waitForExpect = require('wait-for-expect');


This dependency no longer exists, this line can be removed. I believe this is the only reference to waitForExpect.

I found more in tests for useQuery, useRestApi, and magentoRouteHandler.

Yeah, the dependency is still there. Instead of listing it at root I added it to Buildpack and Peregrine, the two places it's used.

supernova-at · 2019-07-31T19:09:53Z

packages/peregrine/lib/Toasts/useToastContext.js

@@ -60,7 +60,8 @@ const reducer = (prevState = initialState, action = {}) => {
    }
 };

-const ToastContext = createContext();
+const ToastContext =
+    window.__ToastContext || (window.__ToastContext = createContext());


Was this creating multiple toast contexts previously?

Yeah, this line confuses me, especially since window.__ToastContext is not referenced anywhere else. If there's a bug with this particular context object, we should diagnose it and fix it.

I don't know how this change made it in here: must have been a bad old rebase. Removed.

supernova-at · 2019-07-31T19:11:59Z

packages/pwa-buildpack/envVarDefinitions.json

+                    "name": "DEVELOPER_DEBUG_BUNDLES",
+                    "type": "bool",
+                    "desc": "Build developer-mode bundles without adding hot reloading or launching the dev server. Disables minification and enables all Webpack debug settings for maximum code readability.",
+                    "default": false


Is this just developer mode?

Also, if I set this to true, yarn watch:all and yarn watch:venia no longer serve properly. Not sure what's going on.

Yeah, it's busted because of some bad choices in the informed library.

supernova-at · 2019-07-31T19:22:19Z

packages/pwa-buildpack/lib/Utilities/configureHost.js

            clearTimeout(timeout);
            resolve({
                key: certBuffers.key.toString('utf8'),
                cert: certBuffers.cert.toString('utf8')
            });
        } catch (e) {
            clearTimeout(timeout);
+            debug('failure! cleared UI timeout for getCert("%s")', hostname);
+            debug(e);


Seems like there are some debug calls you can clear out of here if you want.

I joined those two together, but debug calls are cheap and they should be plentiful.

supernova-at · 2019-07-31T19:34:42Z

packages/pwa-buildpack/lib/Utilities/__tests__/loadEnvironment.spec.js

        TRAPPING_MONK: 'level 1',
        TRAPPING_ELF: 'level 2',
        GEWGAW_PALADIN: 'level 3',
-        GEWGAW_ROGUE: 'level 4',
-        MUSTANG_SALLY: 'level a million'
+        GEWGAW_ROGUE: 'level 4'


This file is hilarious lol

supernova-at · 2019-07-31T20:12:12Z

packages/pwa-buildpack/lib/WebpackTools/configureWebpack.js

+            }),
+            new UpwardIncludePlugin({
+                upwardDirs: hasFlag('upward')
+            }),


I was kind of expecting us not to include the UpwardIncludePlugin at all if hasFlag('upward') is false.
Does the plugin fail gracefully without any upwardDirs?

I was kind of expecting us not to include the UpwardIncludePlugin at all if hasFlag('upward') is false.

Minor correction: hasFlag("upward") can't be false; it would return [] if no packages had that flag.

@supernova-at Good catch! We should always require an upward file in the app itself.

supernova-at · 2019-07-31T20:34:21Z

packages/pwa-buildpack/lib/WebpackTools/plugins/__tests__/UpwardDevServerPlugin.spec.js

-const UpwardPlugin = require('../UpwardPlugin');
+const UpwardDevServerPlugin = require('../UpwardDevServerPlugin');
+
+const mockContext = require('path').resolve(__dirname, '../../../../../../');


This is the path to our project root I'm guessing? If so, may want to drop a comment or rename the variable projectRoot or something

A comment would suffice, I think.

Comment added.

supernova-at · 2019-07-31T20:51:22Z

pwa-devdocs/_drafts/configure-webpack/index.md

+
+### `ConfigureWebpackOptions
+
+TODO


Just highlighting this

I'd like to see this filled out before we merge. At least copy in what was written in the PR description :)

Agreed. I'll update you.

supernova-at · 2019-07-31T20:51:27Z

pwa-devdocs/_drafts/configure-webpack/index.md

+
+## Example
+
+TODO


Just highlighting this

supernova-at · 2019-07-31T20:52:47Z

pwa-devdocs/src/venia-pwa-concept/features/modular-components/index.md

@@ -78,13 +78,37 @@ The Venia Adapter wraps around Venia components to satisfy any implicit external

 ### Venia drivers

+The [`@magento/venia-drivers`][] dependency is a _virtual module_ for centralizing the external dependencies of Venia components that use them, such as GraphQL clients and Redux stores.
+Instead of importing these dependencies directly, Venia components import them from `src/drivers`.


Is this repeating L92 & L93?

Can we drop L92 & L93?

Since @jcalcaben wrote L92 and L93 I'd rather drop these lines, which I've done.

jimbo

18/72 files viewed. Left some comments for now.

jimbo · 2019-07-31T22:36:24Z

packages/babel-preset-peregrine/package.json

+    "@babel/preset-env": "^7.3.4",
+    "@babel/runtime": "^7.4.2",
+    "babel-plugin-dynamic-import-node": "^2.2.0",
+    "babel-plugin-graphql-tag": "^2.0.0"


Was this change (all of these dependencies moving from tilde to caret) intentional?

No, this was a mistake. Reverted!

jimbo · 2019-07-31T22:48:11Z

packages/peregrine/package.json

-  "jsnext:main": "lib/index.js",
-  "es2015": "lib/index.js",
-  "sideEffects": false
+  "esnext": "lib/index.js",


Does this supersede jsnext:main?

The precedence order is in MagentoResolver, under browserFields.

The original idea was to support proposals for this convention from several different projects, for maximum compatibility. Rollup, SystemJS, etc. I think that's beyond scope, so we can just set our own aliases.

jimbo · 2019-07-31T23:35:44Z

packages/pwa-buildpack/lib/WebpackTools/MagentoResolver.js

            modules: [options.paths.root, 'node_modules'],
            mainFiles: ['index'],
-            extensions: ['.mjs', '.js', '.json', '.graphql']
+            mainFields: ['esnext', 'es2015', 'module', 'browser', 'main'],
+            extensions: ['.wasm', '.mjs', '.js', '.json', '.graphql']


IIRC, one optimization to make webpack resolve a lot faster is to cut this field down to just [.js], precluding the need to resolve them in order. I'm surprised we aren't already doing it, actually; could we do it here?

Not only are the other extensions rarely imported, we already tend to include them explicitly for clarity's sake.

// safe to assume `./foo` refers to `foo.js` or `foo/index.js` import Foo from "./foo" // by explicitly appending `.css`, we avoid ambiguity or surprise import defaultClasses from "./foo.css"

I'm not seeing that advantage reliably when I try that. With no cache it's always around 50 seconds, and with full cache it's always around 20.

sirugh

Also leaving evening feedback so far. Still have more to look at.

sirugh · 2019-07-31T16:56:42Z

packages/peregrine/lib/Toasts/useToastContext.js

@@ -60,7 +60,8 @@ const reducer = (prevState = initialState, action = {}) => {
    }
 };

-const ToastContext = createContext();
+const ToastContext =
+    window.__ToastContext || (window.__ToastContext = createContext());


Why is this necessary? I feel like you told me you worked this out in the past and didn't have to do this.

Wasn't even supposed to be here, I've removed it :)

sirugh · 2019-07-31T17:09:46Z

packages/pwa-buildpack/lib/__tests__/cli-load-env.spec.js

+        parsed: {}
+    });
+    loadEnvCliBuilder.handler({ directory: '.' });
+    expect(process.exit).toHaveBeenCalled();


Tests in aws pipeline are failing on this assertion. Can you investigate?

sirugh · 2019-07-31T18:30:53Z

packages/upward-js/lib/ResolverVisitor.js

@@ -5,7 +5,8 @@ const { zipObject } = require('lodash');
 const isPrimitive = require('./isPrimitive');

 class ResolverVisitor {
-    constructor(io, rootDefinition, context) {
+    constructor(upwardPath, io, rootDefinition, context) {


Technically breaking change to upward-js, right?

I noticed we are modifying lots of input args. In general I think it's safer to append new arguments instead of prepend.

Also, some docs on this file would go a long way :D

Yeah, I suppose this shouldn't be a prepend :/ I'll refactor it

sirugh · 2019-07-31T19:37:42Z

packages/venia-concept/webpack.config.js

-
-        // A DevServer generates its own unique output path at startup. It needs
-        // to assign the main outputPath to this value as well.
+            // '@magento/venia-library': {


placeholder?

For demo purposes.

packages/pwa-buildpack/package.json

Co-Authored-By: Jimmy Sanford <jimbo@users.noreply.github.com>

zetlen · 2019-08-07T20:31:31Z

Hey @jcalcaben, I'd like your overt approval before merging this.

jcalcaben

Doc changes and drafts look good to me

zetlen requested review from sirugh, jimbo, jcalcaben and supernova-at July 31, 2019 14:04

zetlen changed the base branch from develop to zetlen/v4api/p1-use-relative-imports July 31, 2019 14:06

PWAStudioBot added pkg:babel-preset-peregrine pkg:graphql-cli-validate-magento-pwa-queries pkg:peregrine pkg:pwa-buildpack pkg:upward-js Pertains to upward-js reference implementation of UPWARD. pkg:upward-spec Pertains to UPWARD specification package. pkg:venia-concept labels Jul 31, 2019

zetlen mentioned this pull request Jul 31, 2019

V4P3: refactor: Move most Venia components to separate UI library #1499

Merged

5 tasks

zetlen changed the title ~~feat(buildpack): Add Webpack configurator facade~~ V4P2: feat(buildpack): Add Webpack configurator facade Jul 31, 2019

zetlen force-pushed the zetlen/v4api/p2-webpack-facade branch from 94383d6 to 6733cec Compare July 31, 2019 15:27

vercel bot had a problem deploying to staging July 31, 2019 15:27 Failure

zetlen mentioned this pull request Jul 31, 2019

V4P1: chore: remove root-relative imports from venia code #1497

Merged

5 tasks

zetlen force-pushed the zetlen/v4api/p2-webpack-facade branch from 6733cec to 87edf4d Compare July 31, 2019 15:39

vercel bot had a problem deploying to staging July 31, 2019 15:39 Failure

zetlen force-pushed the zetlen/v4api/p2-webpack-facade branch from 87edf4d to 984ef97 Compare July 31, 2019 16:12

vercel bot deployed to staging July 31, 2019 16:12 View deployment

supernova-at reviewed Jul 31, 2019

View reviewed changes

supernova-at self-assigned this Jul 31, 2019

cherdman added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Jul 31, 2019

jimbo self-assigned this Jul 31, 2019

jimbo reviewed Jul 31, 2019

View reviewed changes

sirugh reviewed Aug 1, 2019

View reviewed changes

vercel bot deployed to staging August 6, 2019 20:12 View deployment

vercel bot temporarily deployed to staging August 6, 2019 21:34 Inactive

fixup change cli process handling

e7fc5a9

zetlen force-pushed the zetlen/v4api/p2-webpack-facade branch from e96f0ed to e7fc5a9 Compare August 6, 2019 21:36

vercel bot deployed to staging August 6, 2019 21:37 View deployment

fixup tests

35df9e5

vercel bot deployed to staging August 7, 2019 10:12 View deployment

fixup API updates and PR feedback

6bc114e

vercel bot deployed to staging August 7, 2019 11:21 View deployment

fixup test coverage

9309beb

vercel bot deployed to staging August 7, 2019 12:52 View deployment

fixup prettier

466c2ca

vercel bot deployed to staging August 7, 2019 13:01 View deployment

docs: document configureWebpack

b4223a9

vercel bot deployed to staging August 7, 2019 13:25 View deployment

zetlen mentioned this pull request Aug 7, 2019

[feature]: Make Venia components less dependent on specific Webpack config #1502

Closed

6 tasks

zetlen requested a review from jimbo August 7, 2019 13:33

jimbo previously approved these changes Aug 7, 2019

View reviewed changes

packages/pwa-buildpack/package.json Outdated Show resolved Hide resolved

Update packages/pwa-buildpack/package.json

f6f3909

Co-Authored-By: Jimmy Sanford <jimbo@users.noreply.github.com>

zetlen dismissed stale reviews from jimbo via f6f3909 August 7, 2019 20:30

vercel bot deployed to staging August 7, 2019 20:30 View deployment

jcalcaben approved these changes Aug 7, 2019

View reviewed changes

dpatil-magento merged commit 4db08e0 into develop Aug 8, 2019

This was referenced Aug 9, 2019

[bug]: Unable to create subfolders inside queries folder #1108

Closed

[feature]: Split Venia codebase into top-layer app and component library #1551

Closed

supernova-at mentioned this pull request Aug 19, 2019

Venia UI Storybook is broken #1577

Closed

dpatil-magento mentioned this pull request Aug 28, 2019

[bug]: Venia storefront not accessible on cloud starter. Cause PR #1498 #1614

Closed

dpatil-magento deleted the zetlen/v4api/p2-webpack-facade branch September 14, 2020 22:03


		## Example

		TODO

V4P2: feat(buildpack): Add Webpack configurator facade #1498

V4P2: feat(buildpack): Add Webpack configurator facade #1498

Conversation

zetlen commented Jul 31, 2019 • edited

Description

Verification Steps

Proposed Labels for Change Type/Package

Checklist:

vercel bot commented Jul 31, 2019 • edited

PWAStudioBot commented Jul 31, 2019 • edited

coveralls commented Jul 31, 2019 • edited

supernova-at Jul 31, 2019 • edited

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

sirugh Aug 1, 2019 • edited

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

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 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

jimbo Jul 31, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetlen commented Aug 7, 2019

jcalcaben left a comment

Choose a reason for hiding this comment

zetlen commented Jul 31, 2019 •

edited

vercel bot commented Jul 31, 2019 •

edited

PWAStudioBot commented Jul 31, 2019 •

edited

coveralls commented Jul 31, 2019 •

edited

supernova-at Jul 31, 2019 •

edited

sirugh Aug 1, 2019 •

edited

jimbo Jul 31, 2019 •

edited