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

Gracefully handle common HTML attributes when they don't exist as props on component (e.g. id or class) during dev #53

Closed
patricknelson opened this issue Apr 23, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@patricknelson
Copy link
Owner

patricknelson commented Apr 23, 2024

Describe the problem

Right now, even though svelte-retag should already already know all valid props ahead of time, it will still dutifully forward attributes that it is not aware of. That even includes ones which may have no relevance to the component itself and may purely only be useful in HTML, for example:

  • class - useful for styling/positioning the host element
  • id - useful for linking to the host element (e.g. example.com/#element-name)
  • data-* attributes which may be exposed for unit testing purposes

This results in a large number of console warnings similar to this (screenshots below):

was created with unknown prop 'id'

image
image

Describe the proposed solution

It would be cool if svelte-retag had a way of ignoring these warnings without being too heavy handed. Some developers may actually want some of these attributes to be passed along. Warnings in that case would be super useful (reminding them that they may have accidentally forgotten to the id prop to their component).

However, in many situations like those noted above, this may also be annoying. Especially if you want to use a custom element as a page anchor that you can link to or want to position it with a CSS class (but don't have any reason to use that CSS class in the component itself).

Some solutions:

  1. Ignore common HTML attributes if all conditions below are satisfied:
    • attributes: true is specified (i.e. user is just auto forwarding all attributes without explicitly listing them)
    • The attribute is a common one matching the following pattern, either: id, class or data-*
    • svelte-retag has not detected this prop/attribute (i.e. via the proxy which infers all props on the first render)
  2. Same as above, but add the condition:
    • A new option for ignoring warnings when common HTML attributes are set but not present on the component, e.g. ignoreCommonAttribWarnings: false.
    • Disabled by default but can be enabled in situations where you find the error annoying you.
    • Can set to true to allow svelte-retag to decide which attributes to ignore (i.e. the list above) or can set to an array so the developer can tell it to only ignore the desired attributes.
    • Should it accept a callback as well so the developer can decide or is that making things too complicated?

Option 1 feels better because it's more direct, but it does change the functionality of attributes: true slightly. It could confuse users if they expected <example-element id=...> to trigger a warning if the user didn't have the export let id; defined in their component. The biggest risk here though is likely going to be for existing svelte-retag users who may update, but not realize attributes: true has changed to now skip forwarding some attributes if they don't also exist already on the component.

On the flip side, 2 is good because it's a little more careful, but if not done right it could confuse people who expect it to ignore common HTML attributes all the time (which that isn't going to be the case; only if it isn't also present). 🤔

Alternatives considered

Ignore the warnings during development. They do not occur in the production builds.

Importance

nice to have

@patricknelson patricknelson added the enhancement New feature or request label Apr 23, 2024
@patricknelson
Copy link
Owner Author

patricknelson commented Apr 23, 2024

Relates somewhat to #36, #13 and #44. Since attributes: true option is no longer present (and essentially true by default unless props are defined, I think).

So, leaning toward option 2 as being better because theoretically A.) It can be easily done now and not be a breaking change and B.) in svelte-retag@2 when all attributes are automatically forwarded, more people would be likely to be affected, so that'd be safer (i.e. to just assume the developer wants to be nagged). At least for now.

patricknelson added a commit that referenced this issue Apr 23, 2024
…t already defined on the component. Need unit tests...
patricknelson added a commit that referenced this issue May 3, 2024
…re also not already defined on the component (while in development mode).
@patricknelson
Copy link
Owner Author

Fixed in #54.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant