github / github/catalyst

@attr has discrepencies with `attributeChangedCallback`

Open
#117 4 comments 1 reaction 0 assignees View on GitHub
@attr
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.