[PWA-355]: Add richer extension points and documentation #2298

zetlen · 2020-03-31T15:18:18Z

Description

Add features to the extensibility framework to support rich integration with Peregrine talons. This enables extensions that can deeply integrate analytics and segmentation code.

Features

Changed Buildpack wrapEsmModules target to a lower-level, more versatile transformModules target
- Original module-wrap functionality is still available, using a transform of type source and with transformModule wrap-esm-loader.
- New transform type babel allows for AST manipulation.
- Interceptors of transformModules can only intercept their own files.
Added Peregrine talons target for wrapping talon functions
- Only App.useApp and ProductFullDetail.useProductFullDetail implemented so far
- Infinitely composable
Added VeniaUI routes target for adding custom routes
- Refactored Venia's existing custom routes to dogfood this target
Added a suite of test helpers for developers making their own extensions
- Mock builds and build buses for simulating a full build with codemods
- Mock targets for testing intercept behavior
- Mock Webpack loaders for testing source code transforms

Changes

Refactored MagentoResolver to support some common development scenarios
- Symlinked packages
- Packages referencing each other from targets
- Resolving a file path using Webpack rules, before running the Webpack compiler
- Legacy behavior still supported
Refactored configureWebpack into smaller modules for creating different parts of Webpack config
- Test helpers can now simulate a closer-to-real Webpack config by composing these together
- transformModules target implementation now encapsulated in JS module rules
Upgraded Jest to v25
- Performance and snapshot reliability were taking a dive with the extra tests
- Changed some harnesses to be compatible with v25
- Changed some snapshots to be more tolerant of versioning

Docs

Refactored several Buildpack modules to be more readable structures
Put JSDoc comments on everything
- Properly formatted JSDoc comments will enable intellisense in VSCode and IntelliJ environments
- @jcalcaben may also be able to generate formatted documentation from the JSDoc comments
Wrote an introductory developer and user tutorial for Targets

Related Issue

Closes #PWA-355.

Acceptance

Verification Stakeholders

@magento/pwa-studio-team

Verification Steps

There is a step-by-step tutorial in the getting started guide! This can serve as a verification repro.
Quick Start

Proceed through the steps and verify that you can create a new custom route from within the repository.

Additional verification: create a separate package to implement the same functionality as a module. See Extension Development

Checklist

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

PWAStudioBot · 2020-03-31T15:32:27Z

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

Generated by 🚫 dangerJS against 186b3af

zetlen · 2020-04-21T17:53:56Z

.gitignore

+
+## exception: commit node_modules folders in test fixtures
+!**/__fixtures__/*/node_modules


Testing the test helpers meant adding fixtures with node_modules directories.

zetlen · 2020-04-21T17:54:16Z

jest.config.js

-        '\\.svg$': 'identity-obj-proxy'
+        '\\.svg$': 'identity-obj-proxy',
+        '@magento/venia-drivers':
+            '<rootDir>/packages/venia-ui/lib/drivers/index.js'


Many of these small changes were for Jest 25 (see below)

Actually Jest 26 now

package.json

…zetlen/talon-targets

Changes discussed. I think Zetlen has created necessary followups.

…zetlen/talon-targets

dpatil-magento · 2020-05-20T16:00:11Z

Thanks @zetlen , tests looks good. CI Build and Deploy will be fixed by #2406

devops-pwa-codebuild · 2020-05-21T19:23:24Z

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2298.pwa-venia.com : LH Performance Expected 0.85 Actual 0.58, LH Best Practices Expected 1 Actual 0.92
https://pr-2298.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.35, LH Best Practices Expected 1 Actual 0.92
https://pr-2298.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.5, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92

…-studio into zetlen/talon-targets

jimbo

@zetlen I like this approach, and it's nice to see this PR using the new API it introduces to implement some of the things we're already comfortable with, such as routes. It's also great to see all the tests accompanying all the additions and modifications here.

This is an enormous amount of code to add at once, though; it's difficult to review all of this code carefully, and doing so would delay it by a fair bit longer. So I will note that while it's a huge step forward, it is also adding a fair bit of new risk—I can validate the approach and trust in the tests & QA, but ultimately I just have to hope it works.

Let's keep a close eye on how the first few integrations go.

jimbo · 2020-05-18T18:07:46Z

packages/pagebuilder/lib/ContentTypes/ButtonItem/__tests__/buttonItem.spec.js

+afterEach(() => {
+    window.location = oldWindowLocation;
+});
+


This is...gross. If I were writing the unit test for this effect, I would sooner give up than do this kind of mocking. Is there something earlier in the chain we can mock, if window.location is going to be this bad?

I don't like it, but it's the only approach I've seen in extensive research on the subject. It's not that unusual, though. It belongs to a whole class of problems mocking globals in JSDOM.

jimbo · 2020-05-18T19:15:34Z

packages/peregrine/lib/targets/__tests__/peregrine-targets.spec.js

+
+test('enables third parties to wrap talons', async () => {
+    // sorry, buildModuleWith is slow. TODO: make it take less than a minute?
+    jest.setTimeout(60000);


I agree, this is not great and could be improved. At the same time, I'm not sure how much we want to optimize for actual developers blindly running all tests. I think running all tests at once should be avoided, generally, when dealing with a monorepo. But it will happen sometimes, so it probably shouldn't take forever.

As Zetlen said, jest --watch is the recommended approach. I go a step further and input the exact tests I want to run and the exact directories from which I want to collect coverage, which is easy to do with yarn test or yarn jest.

jimbo · 2020-05-18T21:56:41Z

packages/pwa-buildpack/lib/BuildBus/BuildBus.js

+        if (this._hasRun[phase]) {
+            return;
+        }
+        this._hasRun[phase] = true;


Minor suggestion: cases like this are good for a Set, since you're just adding arbitrary strings and then checking for their presence.

this._hasRun = new Set() if (this._hasRun.has(phase) return this._hasRun.add(phase)

It's true, though I think it's a micro-optimization, since the number of phases is unlikely to get much larger.

jimbo · 2020-05-22T21:03:01Z

packages/pwa-buildpack/lib/TestHelpers/testTargets/MockedBuildBus.js

+    /** @public */
+    static clear() {
+        throw new Error(
+            'MockedBuildBus.clear() not supported. More details at https://twitter.com/JamesZetlen/status/1244683087839137792'


Yeah, the only tweet I've ever linked in a code comment has since been deleted.

jimbo · 2020-05-22T21:08:19Z

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

-                                    }
+                            for (const filename of compilation.fileDependencies) {
+                                if (filename.endsWith('.graphql')) {
+                                    queryFilePaths.push(filename);


What a coincidence! #2420

jimbo · 2020-05-22T21:21:31Z

packages/venia-ui/lib/targets/BabelRouteInjectionPlugin.js

+                                    )}`
+                                );
+                            }
+                            // if we got here, both have been seen, so this is an exact duplicate;


When we're discussing routes here, we're discussing paths to a module ("Babel routes"), correct?

Not URL patterns?

jimbo · 2020-05-22T21:22:42Z

packages/venia-ui/lib/targets/__tests__/__fixtures__/project/fakePlainHtmlRenderer.js

@@ -0,0 +1,3 @@
+export const Component = ({ html }) => `I declare ${html}`;


I declare bankruptcy!

…zetlen/talon-targets

dpatil-magento · 2020-06-01T21:27:07Z

@zetlen yarn build is failing with error

yarn run v1.13.0
$ yarn venia run build
$ yarn workspace @magento/venia-concept run build
$ yarn run build:prod
$ yarn run clean && webpack --no-progress --env.mode production
$ rimraf dist
Error: [pertain] "@magento/venia-concept" declares a "pwa-studio.targets.intercept" module, but Node could not find a valid JS module to load from "../pwa-studio/packages/venia-concept/interceptlocal.js"

…-studio into zetlen/talon-targets

m2-community-project bot added this to Ready for Review in Pull Request Progress Mar 31, 2020

zetlen changed the title ~~Zetlen/talon targets~~ [PWA-355]: Add extension points to support analytics across the site Mar 31, 2020

zetlen added extensibility extension framework pkg:peregrine pkg:pwa-buildpack tooling Related to the developer tooling, including buildpack, webpack plugins, and debug UIs. version: Minor This changeset includes functionality added in a backwards compatible manner. labels Mar 31, 2020

PWAStudioBot added pkg:pagebuilder pkg:venia-concept pkg:venia-ui labels Mar 31, 2020

zetlen force-pushed the zetlen/talon-targets branch 2 times, most recently from fd0d1f7 to 3022c67 Compare April 10, 2020 11:47

PWAStudioBot added pkg:babel-preset-peregrine pkg:graphql-cli-validate-magento-pwa-queries pkg:upward-js Pertains to upward-js reference implementation of UPWARD. pkg:upward-spec Pertains to UPWARD specification package. pkg:venia-styleguide labels Apr 10, 2020

zetlen force-pushed the zetlen/talon-targets branch 5 times, most recently from a256539 to 9ad094a Compare April 14, 2020 19:13

zetlen force-pushed the zetlen/talon-targets branch from ce34273 to a64d3a2 Compare April 21, 2020 17:00

zetlen commented Apr 21, 2020

View reviewed changes

m2-community-project bot assigned zetlen Apr 21, 2020

m2-community-project bot moved this from Ready for Review to Review in Progress in Pull Request Progress Apr 21, 2020

m2-community-project bot added the Progress: review label Apr 21, 2020

zetlen commented Apr 21, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

James Zetlen added 5 commits May 11, 2020 11:44

fixup var names i missed

68b95e3

Merge branch 'develop' of https://github.com/magento/pwa-studio into …

396f747

…zetlen/talon-targets

Merge branch 'develop' into zetlen/talon-targets

8ee376d

Merge branch 'develop' into zetlen/talon-targets

df1277f

Merge branch 'develop' of https://github.com/magento/pwa-studio into …

1e2f6a8

…zetlen/talon-targets

m2-community-project bot assigned jimbo May 14, 2020

Merge branch 'develop' of https://github.com/magento/pwa-studio into …

a59fceb

…zetlen/talon-targets

dpatil-magento mentioned this pull request May 19, 2020

update docker image #2406

Merged

James Zetlen added 2 commits May 19, 2020 15:57

fixup update snapshots after merge

ec8fd6c

Merge branch 'develop' of https://github.com/magento/pwa-studio into …

6020571

…zetlen/talon-targets

James Zetlen and others added 2 commits May 20, 2020 18:20

Merge branch 'develop' into zetlen/talon-targets

bd6d470

Merge branch 'develop' into zetlen/talon-targets

c8e240b

James Zetlen added 2 commits May 21, 2020 20:23

fixup watch server regression

5f4d98e

Merge branch 'zetlen/talon-targets' of https://github.com/magento/pwa…

8a06315

…-studio into zetlen/talon-targets

jimbo suggested changes May 22, 2020

View reviewed changes

Merge branch 'develop' of https://github.com/magento/pwa-studio into …

cc448c4

…zetlen/talon-targets

jimbo previously approved these changes Jun 1, 2020

View reviewed changes

Merge branch 'develop' into zetlen/talon-targets

fdb35df

James Zetlen added 2 commits June 1, 2020 16:38

fixup add local intercept back

53173a8

Merge branch 'zetlen/talon-targets' of https://github.com/magento/pwa…

186b3af

…-studio into zetlen/talon-targets

zetlen dismissed jimbo’s stale review via 186b3af June 1, 2020 21:41

zetlen merged commit 1b325a2 into develop Jun 2, 2020

m2-community-project bot moved this from Changes Requested to Done in Pull Request Progress Jun 2, 2020

m2-community-project bot removed the Progress: changes requested label Jun 2, 2020

This was referenced Jun 2, 2020

Revert "[PWA-355]: Add richer extension points and documentation" #2446

Merged

[PWA-355]: Add richer extension points and documentation #2451

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PWA-355]: Add richer extension points and documentation #2298

[PWA-355]: Add richer extension points and documentation #2298

zetlen commented Mar 31, 2020 •

edited

PWAStudioBot commented Mar 31, 2020 •

edited

zetlen Apr 21, 2020

zetlen Apr 21, 2020

zetlen May 7, 2020

dpatil-magento commented May 20, 2020

devops-pwa-codebuild commented May 21, 2020 •

edited

jimbo left a comment

jimbo May 18, 2020

zetlen May 30, 2020

jimbo May 18, 2020

jimbo May 18, 2020

zetlen May 30, 2020

jimbo May 22, 2020

jimbo May 22, 2020

zetlen May 30, 2020

jimbo May 22, 2020

jimbo May 22, 2020

dpatil-magento commented Jun 1, 2020


		## exception: commit node_modules folders in test fixtures
		!*/__fixtures__//node_modules

		@@ -0,0 +1,3 @@
		export const Component = ({ html }) => `I declare ${html}`;

[PWA-355]: Add richer extension points and documentation #2298

[PWA-355]: Add richer extension points and documentation #2298

Conversation

zetlen commented Mar 31, 2020 • edited

Description

Features

Changes

Docs

Related Issue

Acceptance

Verification Stakeholders

Verification Steps

Checklist

PWAStudioBot commented Mar 31, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpatil-magento commented May 20, 2020

devops-pwa-codebuild commented May 21, 2020 • edited

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

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

dpatil-magento commented Jun 1, 2020

zetlen commented Mar 31, 2020 •

edited

PWAStudioBot commented Mar 31, 2020 •

edited

devops-pwa-codebuild commented May 21, 2020 •

edited