-
Notifications
You must be signed in to change notification settings - Fork 683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PWA-355]: Add richer extension points and documentation #2298
Changes from all commits
6f61fa3
8a19226
7ab6808
c5114f0
9486131
d34cc2b
b30e790
2e813a6
75cba6c
9857a5f
6068ed2
a9ccfcf
ef159e7
33abe89
4fe3789
be01e64
abf01e2
a51e977
62eeffd
37c79c2
6629515
6435361
68b95e3
396f747
8ee376d
df1277f
1e2f6a8
a59fceb
ec8fd6c
6020571
bd6d470
c8e240b
5f4d98e
8a06315
cc448c4
fdb35df
53173a8
186b3af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,7 +42,6 @@ const testGlob = '/**/{src,lib,_buildpack}/**/__tests__/*.(test|spec).js'; | |
// Reusable test configuration for Venia UI and storefront packages. | ||
const testReactComponents = inPackage => ({ | ||
// Expose jsdom to tests. | ||
browser: true, | ||
moduleNameMapper: { | ||
// Mock binary files to avoid excess RAM usage. | ||
'\\.(jpg|jpeg|png)$': | ||
|
@@ -52,7 +51,9 @@ const testReactComponents = inPackage => ({ | |
// This mapping forces CSS Modules to return literal identies, | ||
// so e.g. `classes.root` is always `"root"`. | ||
'\\.css$': 'identity-obj-proxy', | ||
'\\.svg$': 'identity-obj-proxy' | ||
'\\.svg$': 'identity-obj-proxy', | ||
'@magento/venia-drivers': | ||
'<rootDir>/packages/venia-ui/lib/drivers/index.js' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Many of these small changes were for Jest 25 (see below) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually Jest 26 now |
||
}, | ||
moduleFileExtensions: ['ee.js', 'ce.js', 'js', 'json', 'jsx', 'node'], | ||
// Reproduce the Webpack resolution config that lets Venia import | ||
|
@@ -73,7 +74,7 @@ const testReactComponents = inPackage => ({ | |
// import `.graphql` files into JS. | ||
'\\.(gql|graphql)$': 'jest-transform-graphql', | ||
// Use the default babel-jest for everything else. | ||
'\\.(js|css)$': 'babel-jest' | ||
'\\.(jsx?|css)$': 'babel-jest' | ||
}, | ||
// Normally babel-jest ignores node_modules and only transpiles the current | ||
// package's source. The below setting forces babel-jest to transpile | ||
|
@@ -256,7 +257,6 @@ const jestConfig = { | |
configureProject('pagebuilder', 'Pagebuilder', testReactComponents), | ||
configureProject('peregrine', 'Peregrine', inPackage => ({ | ||
// Expose jsdom to tests. | ||
browser: true, | ||
setupFiles: [ | ||
// Shim DOM properties not supported by jsdom | ||
inPackage('scripts/shim.js'), | ||
|
@@ -318,6 +318,7 @@ const jestConfig = { | |
// Not node_modules | ||
'!**/node_modules/**', | ||
// Not __tests__, __helpers__, or __any_double_underscore_folders__ | ||
'!**/TestHelpers/**', | ||
'!**/__[[:alpha:]]*__/**', | ||
'!**/.*/__[[:alpha:]]*__/**', | ||
// Not this file itself | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,7 +39,7 @@ | |
"storybook:venia": "yarn workspace @magento/venia-ui run storybook", | ||
"test": "jest", | ||
"test:ci": "jest --no-cache --max-workers=3 --json --outputFile=test-results.json", | ||
"test:debug": "node --inspect-brk node_modules/.bin/jest --no-cache --no-coverage --runInBand", | ||
"test:debug": "node --inspect-brk node_modules/.bin/jest --no-cache --no-coverage --runInBand --testTimeout 86400", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Default is 5000m - did we need to extend the timeout for certain tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, see below. The test helpers are slow, and I don't think it's easily fixable; we should probably talk about putting them in a separate integration test suite. |
||
"test:dev": "jest --watch", | ||
"validate-queries": "yarn venia run validate-queries", | ||
"venia": "yarn workspace @magento/venia-concept", | ||
|
@@ -48,7 +48,7 @@ | |
}, | ||
"devDependencies": { | ||
"@magento/eslint-config": "~1.5.0", | ||
"@types/jest": "~24.0.18", | ||
"@types/jest": "~25.2.1", | ||
"caller-id": "~0.1.0", | ||
"chalk": "~2.4.2", | ||
"chokidar": "~2.1.2", | ||
|
@@ -68,9 +68,9 @@ | |
"first-run": "~2.0.0", | ||
"graphql-tag": "~2.10.1", | ||
"identity-obj-proxy": "~3.0.0", | ||
"jest": "~24.3.1", | ||
"jest": "~26.0.1", | ||
"jest-fetch-mock": "~2.1.1", | ||
"jest-junit": "~6.3.0", | ||
"jest-junit": "~10.0.0", | ||
"jest-transform-graphql": "~2.1.0", | ||
"lodash.debounce": "~4.0.8", | ||
"prettier": "~1.16.4", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,9 +58,7 @@ test('renders a Block component with all props configured and Page Builder rich | |
}; | ||
const component = createTestInstance(<Block {...blockProps} />); | ||
|
||
expect( | ||
component.root.find(child => child.type.name === 'Row') | ||
).toBeTruthy(); | ||
expect(component.root.find(child => child.type === MockRow)).toBeTruthy(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small change for Jest 25 quirk. |
||
}); | ||
|
||
test('renders a Block component with HTML content', () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,21 @@ jest.mock('@magento/venia-drivers', () => ({ | |
})); | ||
useHistory.mockImplementation(() => history); | ||
|
||
const mockWindowLocation = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Jest 25's |
||
assign: jest.fn() | ||
}; | ||
|
||
let oldWindowLocation; | ||
beforeEach(() => { | ||
oldWindowLocation = window.location; | ||
delete window.location; | ||
window.location = mockWindowLocation; | ||
mockWindowLocation.assign.mockClear(); | ||
}); | ||
afterEach(() => { | ||
window.location = oldWindowLocation; | ||
}); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
test('renders a ButtonItem component', () => { | ||
const component = createTestInstance(<ButtonItem />); | ||
|
||
|
@@ -73,7 +88,6 @@ test('clicking button with external link goes to correct destination', () => { | |
}; | ||
const component = createTestInstance(<ButtonItem {...buttonItemProps} />); | ||
const button = component.root.findByType(Button); | ||
window.location.assign = jest.fn(); | ||
button.props.onClick(); | ||
expect(window.location.assign).toBeCalledWith('https://www.adobe.com'); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,7 +61,7 @@ | |
}, | ||
"pwa-studio": { | ||
"targets": { | ||
"intercept": "./intercept" | ||
"intercept": "./lib/intercept" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rearranged code to make sure that everything is in |
||
} | ||
}, | ||
"sideEffects": false | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
const { | ||
buildModuleWith, | ||
mockTargetProvider | ||
} = require('@magento/pwa-buildpack'); | ||
const declare = require('../peregrine-declare'); | ||
const intercept = require('../peregrine-intercept'); | ||
|
||
test('declares a sync target talons and intercepts transformModules', () => { | ||
const targets = mockTargetProvider( | ||
'@magento/peregrine', | ||
(_, dep) => | ||
({ | ||
'@magento/pwa-buildpack': { | ||
specialFeatures: { | ||
tap: jest.fn() | ||
}, | ||
transformModules: { | ||
tap: jest.fn() | ||
} | ||
} | ||
}[dep]) | ||
); | ||
declare(targets); | ||
expect(targets.own.talons.tap).toBeDefined(); | ||
const hook = jest.fn(); | ||
// no implementation testing in declare phase | ||
targets.own.talons.tap('test', hook); | ||
targets.own.talons.call('woah'); | ||
expect(hook).toHaveBeenCalledWith('woah'); | ||
|
||
intercept(targets); | ||
const buildpackTargets = targets.of('@magento/pwa-buildpack'); | ||
expect(buildpackTargets.transformModules.tap).toHaveBeenCalled(); | ||
}); | ||
|
||
test('enables third parties to wrap talons', async () => { | ||
// sorry, buildModuleWith is slow. TODO: make it take less than a minute? | ||
jest.setTimeout(60000); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we discussed this worries me, how common is it for developers to run all tests during their development cycle? Is there a way we could make some of these tests only run on PR to save developers the minute for this single test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could! It's just not something we have set up currently. We could create a two-tiered test suite for Jest unit tests versus integration tests. In normal workflow, a developer should be using Jest 26 really speeds it up, so I think that the full test suite might get a lot less common. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, |
||
const talonIntegratingDep = { | ||
name: 'goose-app', | ||
declare() {}, | ||
intercept(targets) { | ||
targets.of('@magento/peregrine').talons.tap(talons => { | ||
talons.ProductFullDetail.useProductFullDetail.wrapWith( | ||
'src/usePFDIntercept' | ||
); | ||
talons.App.useApp.wrapWith('src/useAppIntercept'); | ||
talons.App.useApp.wrapWith('src/swedish'); | ||
}); | ||
} | ||
}; | ||
const built = await buildModuleWith('src/index.js', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is one of the test helpers for unit testing extensions. It does a full Webpack build and gives you a component runnable in Node, so you can test how your extension affected the built and generated code. |
||
context: __dirname, | ||
dependencies: [ | ||
{ | ||
name: '@magento/peregrine', | ||
declare, | ||
intercept | ||
}, | ||
talonIntegratingDep | ||
], | ||
mockFiles: { | ||
'src/index.js': ` | ||
import { useApp } from '@magento/peregrine/lib/talons/App/useApp'; | ||
import { useProductFullDetail } from '@magento/peregrine/lib/talons/ProductFullDetail/useProductFullDetail'; | ||
export default useApp() + useProductFullDetail()`, | ||
'src/usePFDIntercept': `export default function usePFDIntercept(original) { return function usePFD() { return 'BEEP >o'; } };`, | ||
'src/useAppIntercept': `export default function useAppIntercept(original) { | ||
return function useApp() { | ||
return 'o< HONK'; | ||
}; | ||
} | ||
`, | ||
'src/swedish': `export default function swedish(impl) { | ||
return function() { | ||
return impl().replace("O", "Ö") | ||
} | ||
}` | ||
} | ||
}); | ||
|
||
expect(built.run()).toBe('o< HÖNKBEEP >o'); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
/** | ||
* These targets are available for interception to modules which depend on `@magento/peregrine`. | ||
* | ||
* Their implementations are found in `./peregrine-intercept.js`. | ||
* | ||
* @module Peregrine/Targets | ||
*/ | ||
module.exports = targets => { | ||
targets.declare({ | ||
/** | ||
* @callback talonIntercept | ||
* @param {Peregrine/Targets.TalonWrapperConfig} talons - Registry of talon namespaces, talons, and Sets of interceptors | ||
* @returns {undefined} - Interceptors of `talons` should add to the | ||
* sets on passed TalonWrapperConfig instance. Any returned value will | ||
* be ignored. | ||
*/ | ||
|
||
/** | ||
* Collects requests to intercept and "wrap" individual Peregrine | ||
* talons in decorator functions. Use it to add your own code to run | ||
* when Peregrine talons are invoked, and/or to modify the behavior and | ||
* output of those talons. | ||
* | ||
* This target is a convenience wrapper around the | ||
* `@magento/pwa-buildpack` target `transformModules`. That target uses | ||
sirugh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* filenames, which are not guaranteed to be semantically versioned or | ||
* to be readable APIs. | ||
* This target publishes talons as functions to wrap, rather than as | ||
* files to decorate. | ||
* | ||
* @type {tapable.SyncHook} | ||
* @param {talonIntercept} | ||
* | ||
* @example <caption>Log whenever the `useApp()` hook runs.</caption> | ||
* targets.of('@magento/peregrine').talons.tap(talons => { | ||
* talons.App.useApp.wrapWith('./log-wrapper'); | ||
* }) | ||
* // log-wrapper.js: | ||
* export default function wrapUseApp(original) { | ||
* return function useApp(...args) { | ||
* console.log('calling useApp with', ...args); | ||
* return original(...args); | ||
* } | ||
* } | ||
*/ | ||
talons: new targets.types.Sync(['talons']) | ||
}); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
/** | ||
* @module Peregrine/Targets | ||
*/ | ||
const path = require('path'); | ||
|
||
/** | ||
* | ||
* @class TalonWrapperConfig | ||
* @hideconstructor | ||
*/ | ||
function TalonWrapperConfig(addTransforms) { | ||
const wrappable = (talonFile, exportName) => ({ | ||
wrapWith: wrapperModule => | ||
addTransforms({ | ||
type: 'source', | ||
fileToTransform: path.join('./lib/talons/', talonFile), | ||
transformModule: | ||
'@magento/pwa-buildpack/lib/WebpackTools/loaders/wrap-esm-loader', | ||
options: { | ||
wrapperModule, | ||
exportName | ||
} | ||
}) | ||
}); | ||
return { | ||
/** | ||
* @memberof TalonWrapperConfig | ||
* | ||
*/ | ||
|
||
ProductFullDetail: { | ||
useProductFullDetail: wrappable( | ||
'ProductFullDetail/useProductFullDetail', | ||
'useProductFullDetail' | ||
) | ||
}, | ||
App: { | ||
useApp: wrappable('App/useApp', 'useApp') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels a bit too verbose. Why do we need so much repetition here around the component and talon names? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The second argument is the name of the export to wrap. All of the talon files export their main talon functions as a named export, so that looks like our convention, but are we sticking to it? If so, we can maybe automate some of what this class does, since its implementation is internal to Peregrine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why we export named talons vs just have them as the default but it may be too late to make such a sweeping change anyways. I'd prefer we not have to provide the export name. I had assumed we would just automate intercepts around all talons by default. @jimbo may have an opinion about this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, it's verbose and repetitive because it's explicit, which is a Good Thing here. We certainly wouldn't just tap into every export (
When you and I wrote these talons, React hooks were new. I elected to make them named exports because I was uncertain that our developer users would grasp the magical requirement that hooks must be named As it turns out, we haven't really had an issue. The React community at large has adopted the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was a very bright idea in the first place. I thought it was general doubt about default exports, but the enforcing of a naming convention was very smart. We can reduce complexity by switching to default exports. I don't think much churn would be required, because you can just put Wanna schedule that, and mark the verbosity in this file as tech debt? It's up to y'all. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, actually. As you're suggesting, if we were to add a default export to the talons, and deprecate but not remove the named export, it wouldn't even require us to bump the major version. export const useFoo = () => {}
export default useFoo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I'll push that as a todo. |
||
} | ||
}; | ||
} | ||
|
||
module.exports = targets => { | ||
const builtins = targets.of('@magento/pwa-buildpack'); | ||
|
||
builtins.specialFeatures.tap(featuresByModule => { | ||
featuresByModule['@magento/peregrine'] = { | ||
cssModules: true, | ||
esModules: true, | ||
graphQlQueries: true | ||
}; | ||
}); | ||
|
||
/** | ||
* Tap the low-level Buildpack target for wrapping _any_ frontend module. | ||
* Wrap the config object in a TalonWrapperConfig, which presents | ||
* higher-level targets for named and namespaced talons, instead of the | ||
* file paths directly. | ||
* Pass that higher-level config through `talons` interceptors, so they can | ||
* add wrappers for the talon modules without tapping the `transformModules` | ||
* config themselves. | ||
*/ | ||
builtins.transformModules.tap(addTransform => { | ||
const talonWrapperConfig = new TalonWrapperConfig(addTransform); | ||
|
||
targets.own.talons.call(talonWrapperConfig); | ||
}); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
module.exports = { | ||
configuredDomains: jest.fn().mockReturnValue([]), | ||
certificateFor: jest.fn().mockReturnValue({ | ||
key: 'fakekey', | ||
cert: 'fakecert' | ||
}) | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
// don't actually word wrap, it messes with tests since the autodetect is | ||
// system-dependent | ||
const wordWrap = jest.requireActual('word-wrap'); | ||
module.exports = (str, opts) => | ||
wordWrap(str, { | ||
...opts, | ||
width: 1000 | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing the test helpers meant adding fixtures with node_modules directories.