-
Notifications
You must be signed in to change notification settings - Fork 328
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
Relax requirements to receive configuration with createAll
#5374
Merged
romaricpascal
merged 1 commit into
public-js-api
from
simplify-createall-config-requirements
Oct 4, 2024
Merged
Relax requirements to receive configuration with createAll
#5374
romaricpascal
merged 1 commit into
public-js-api
from
simplify-createall-config-requirements
Oct 4, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-5374
October 3, 2024 16:09
Inactive
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for b7c90a2 |
JavaScript changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js
index 4cb3bbdca..adc7bd010 100644
--- a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js
+++ b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js
@@ -1175,7 +1175,7 @@ function createAll(Component, t, e) {
const s = i.querySelectorAll(`[data-module="${Component.moduleName}"]`);
return isSupported() ? Array.from(s).map((e => {
try {
- return "defaults" in Component && void 0 !== t ? new Component(e, t) : new Component(e)
+ return void 0 !== t ? new Component(e, t) : new Component(e)
} catch (i) {
return n && i instanceof Error ? n(i, {
element: e,
Action run for b7c90a2 |
Other changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/all.bundle.js b/packages/govuk-frontend/dist/govuk/all.bundle.js
index 92f1e5508..bfe80911f 100644
--- a/packages/govuk-frontend/dist/govuk/all.bundle.js
+++ b/packages/govuk-frontend/dist/govuk/all.bundle.js
@@ -2557,7 +2557,7 @@
}
return Array.from($elements).map($element => {
try {
- return 'defaults' in Component && typeof config !== 'undefined' ? new Component($element, config) : new Component($element);
+ return typeof config !== 'undefined' ? new Component($element, config) : new Component($element);
} catch (error) {
if (onError && error instanceof Error) {
onError(error, {
diff --git a/packages/govuk-frontend/dist/govuk/all.bundle.mjs b/packages/govuk-frontend/dist/govuk/all.bundle.mjs
index 4be703384..037a2f90d 100644
--- a/packages/govuk-frontend/dist/govuk/all.bundle.mjs
+++ b/packages/govuk-frontend/dist/govuk/all.bundle.mjs
@@ -2551,7 +2551,7 @@ function createAll(Component, config, createAllOptions) {
}
return Array.from($elements).map($element => {
try {
- return 'defaults' in Component && typeof config !== 'undefined' ? new Component($element, config) : new Component($element);
+ return typeof config !== 'undefined' ? new Component($element, config) : new Component($element);
} catch (error) {
if (onError && error instanceof Error) {
onError(error, {
diff --git a/packages/govuk-frontend/dist/govuk/init.mjs b/packages/govuk-frontend/dist/govuk/init.mjs
index e77789089..05b0bbfb5 100644
--- a/packages/govuk-frontend/dist/govuk/init.mjs
+++ b/packages/govuk-frontend/dist/govuk/init.mjs
@@ -89,7 +89,7 @@ function createAll(Component, config, createAllOptions) {
}
return Array.from($elements).map($element => {
try {
- return 'defaults' in Component && typeof config !== 'undefined' ? new Component($element, config) : new Component($element);
+ return typeof config !== 'undefined' ? new Component($element, config) : new Component($element);
} catch (error) {
if (onError && error instanceof Error) {
onError(error, {
Action run for b7c90a2 |
Remove the need for components to have a `defaults` static property, as it's really an internal convention which is a lot to ask from outside components.
romaricpascal
force-pushed
the
simplify-createall-config-requirements
branch
from
October 3, 2024 17:01
c48e17d
to
b7c90a2
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-5374
October 3, 2024 17:01
Inactive
patrickpatrickpatrick
approved these changes
Oct 4, 2024
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.
Makes sense to me!
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove the need for components to have a
defaults
static property, as it's an internal convention which is a lot to ask from outside components.Thoughts
Looks like that check was there to help TypeScript follow when the code was inside
initAll
. With the introduction ofCompatibleClass
type and its{new (...args: any[]): any}
constructor for the implementation ofcreateAll
, we no longer need it as theCompatibleClass
loosened the requirements for the components' constructors.An alternative could have been to check the constructor's length with the
length
property ofFunction
before passing a config. This would put a hard constraints on constructors not to use rest parameters (constructor(...args) { }
), as those are not counted.If a call to
createAll
receives aconfig
and a component that does not expect a second argument, no harm done. The only issue would be if a component would expect something else than aconfig
as a second argument but we'll be documenting the shape of constructors in our documentation (not that it prevents people from doing unexpected things, but I think we should have a certain level of trust with people using our API before getting too defensive. If we see too many issues, we can look at educating more or tightening requirements in the future).Closes #5373