-
Notifications
You must be signed in to change notification settings - Fork 319
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
checkSupport
made static
and protected
#5336
base: public-js-api
Are you sure you want to change the base?
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 290af25 |
6c3228e
to
fd1179a
Compare
`checkSupport` of `GOVUKFrontend` can be overloaded by classes that extend it. Added tests for `createAll` when `checkSupport` has been overloaded.
fd1179a
to
290af25
Compare
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.
Little misunderstanding on the static
ness of the checkSupport
method, sorry 😔 I've clarified in the comments 😊
* @protected | ||
* @static | ||
* @throws {SupportError} when the components are not supported | ||
*/ | ||
checkSupport() { |
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.
issue This only documents the method as static
, but the method itself is not static
from a JavaScript point of view (it's interesting that TypeScript does not pick up on that, actually, that'll be something to remember).
* @protected | |
* @static | |
* @throws {SupportError} when the components are not supported | |
*/ | |
checkSupport() { | |
* @protected | |
* @throws {SupportError} when the components are not supported | |
*/ | |
static checkSupport() { |
Having the method static will help avoid overloading the console with errors when a component is repeated multiple times on the page (like the Copy button on the Design System page), letting createAll
do a single initial check before looping.
const result = createAll(MockComponentWithCheckSupport) | ||
expect(checkSupportMock).toHaveBeenCalled() | ||
expect(result).toStrictEqual([]) | ||
expect(global.console.log).toHaveBeenCalledWith(expect.any(Error)) |
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.
suggestion Is there a way we can check this is the error thrown by the support mock (for example, comparing the text of the error)?
class MockComponent extends GOVUKFrontendComponent { | ||
constructor(...args) { | ||
super(...args) | ||
this.args = args | ||
} |
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.
Good spot to update this here 🙌🏻
class MockComponentWithCheckSupport extends MockComponent { | ||
checkSupport() { | ||
checkSupportMock() | ||
} | ||
} |
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.
suggestion Possibly a long shot, but does jest.spyOn
let us avoid creating one class per test with the method being static?
const checkSupportMock = jest.spyOn(MockComponent, 'checkSupport')
What
Specify that
checkSupport
ofGOVUKFrontendComponent
isstatic
andprotected
instead ofprivate
.Why
Change in approach to how we allow people to run their own function to check if a component is supported. This change means now components that extend
GOVUKFrontendComponent
can overloadcheckSupport
(as can be seen in the tests).Fixes #5225 (comment)