@attr has discrepencies with `attributeChangedCallback`
- Dominant language
- TypeScript
- Stars
- 1.4k
- Forks
- 59
- Avg merge
- 22h 12m
- Merged PRs (30d)
- 5
Description
The `attributeChangedCallback` function is called whenever an observed attribute is set, added or removed. The `attributeChangedCallback` is called with the raw values of the attribute name, old value and new value.
This can be surprising when using the `@attr` decorator which gives conveniences over the attributes by allowing for camel-cased names eliding their `data-` prefix, and allowing types other than `string`. It can also be surprising to see `attributeChangedCallback` being called with `null` when you type an `@attr` as `string`.
For example:
```typescript
@controller
class FooBarElement extends HTMLElement {
@attr foo = true
attributeChangedCallback(name, oldValue, newValue) {
console.log(name, oldValue, newValue)
}
}
```
When this element first connects, `attributeChangedCallback('data-foo', null, '')` will fire. `attributeChangedCallback` will never fire with `'foo'` as the name, because that is a Catalyst concept and not a WC concept. The same is true of the `boolean` type; `attributeChangedCallback` will never fire with booleans for values. This means developers have to suddenly work around all of the conveniences `@attr` offers them:
```typescript
@controller
class FooBarElement extends HTMLElement {
@attr foo = true
attributeChangedCallback(name, oldValue, newValue) {
if (name === 'data-foo') { // Smell: `data-foo` is the name of the attribute but we call if `foo` everywhere else in the file
if (newValue === null) { // Smell: this should really be `if (oldValue === false)` to align with our expected types
doStuff();
}
}
}
}
```
### Possible Solutions
1. Document this gotcha and just let developers deal with it.
- This burdens developers with the extra concepts they have to keep in their brain
- It demonstrates that this is a leaky abstraction
- The code they eventually have to write still smells
2. Add extra code to call `attributeChangedCallback` with the mapped `name`, `oldValue`, `newValue`
- Potentially harmful: `attributeChangedCallback` has a fixed signature where it is only called with `string|null` and we're abusing that contract.
- This will cause the `attributeChangedCallback` to fire far more often, effectively double for each change to an `attr` mapped value. Use cases which do not care about the argument values will see more - effectively redundant - calls.
3. Add extra code to call a new function (maybe `attrChangedCallback`) with the mapped `name`, `oldValue`, `newValue`.
- This means further divergence from web component spec.
- More to document, more stuff that Catalyst is responsible for.
4. Add a function which can be given the 3 arguments and map it back to prop names (e.g. `const [realName, realOldValue, realNewValue] = this.attributeToAttr(name, oldValue, newValue)`)
- I hate it
- Still burdens developers with extra concepts
- Still demonstrates the leaky abstraction
- Still requires more documentation, more stuff that Catalyst is responsible for.
5. Add events which are automatically dispatched when attributes changed, which can be listened for. changing `HelloWorldElement`'s `@attr name = ''` value could emit `hello-world-name-changed`.
- It's still idiomatic to web components... sort of
- Elements become very noisy
- Required elements binding to their own events, so they're not in control of their own state.
- Still requires more documentation
Contributor guide
Assessment
This issue has not been assessed yet.