Skip to content
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

Review ElementErrors and tie up any loose ends #4036

Closed
2 of 3 tasks
Tracked by #3478
romaricpascal opened this issue Aug 1, 2023 · 3 comments · Fixed by #4323
Closed
2 of 3 tasks
Tracked by #3478

Review ElementErrors and tie up any loose ends #4036

romaricpascal opened this issue Aug 1, 2023 · 3 comments · Fixed by #4323
Milestone

Comments

@romaricpascal
Copy link
Member

romaricpascal commented Aug 1, 2023

What

Review the MissingElementError work (tracked here: #3478) and tie up any loose ends before a content review.

Why

This is part of our work to have our components throw rather than silently return during initialisation.

Who needs to work on this

Developers

Who needs to review this

Developers

Done when

@romaricpascal romaricpascal added awaiting triage Needs triaging by team javascript and removed awaiting triage Needs triaging by team labels Aug 1, 2023
@romaricpascal romaricpascal added this to the v5.0 milestone Aug 1, 2023
@romaricpascal
Copy link
Member Author

romaricpascal commented Aug 24, 2023

Let's divide this issue into smaller ones (attached to #3478), in order of complexity:

@domoscargin domoscargin changed the title Throw errors during JavaScript initialisation if key HTML elements are missing Review MissingElementErrors and tie up any loose ends Aug 24, 2023
@domoscargin
Copy link
Contributor

From a discussion on slack:

We may want to look at removing the element param from the ElementError and just tell the error what's missing or invalid. We're using it like a boolean, and often just pass in null.

@romaricpascal
Copy link
Member Author

romaricpascal commented Sep 28, 2023

Maybe it's a sign that we need to have the ElementError just accept a generic string message and move the formatting to separate helper functions rather than pass that boolean-ish first param: missingElementMessage and missingOrMistypedElementMessage.

I'd like to propose something like the following (the passing of the componentName being a parallel concern as it affects multiple errors, we can always add in an extra options parameter to the constructor anyway or with a componentErrorMessage function to wrap other message).

class ElementError extends Error {
  name = 'ElementError'
}

function missingMessage(identifier) {
  return `${identifier} is missing`  
}

// Two little updates here:
// - identifier as first param to keep in line with the `missingElementMessage
//
function missingOrMistypedMessage(identifier, {value, expectedType = HTMLElement.name}) {
  if (!value) return missingElementMessage(identifier)

  return `${identifier} is not of the type ${expectedType}`
}

throw new ElementError(missingMessage('button'));
throw new ElementError(missingOrMistypedMessage('input', {value: $input});

Depends on how we want to word things for #4072, and is an internal refactoring so could wait if necessary.

@domoscargin domoscargin changed the title Review MissingElementErrors and tie up any loose ends Review ElementErrors and tie up any loose ends Oct 24, 2023
@colinrotherham colinrotherham linked a pull request Oct 24, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

2 participants