[FEAT] Modifier Managers #245
Conversation
pzuraq
force-pushed
the
feat/modifier-managers
branch
from
fb70b9a
to
793a5d3
Compare
Adds modifier managers that match Ember's modifier manager API. They can
be defined by importing `setModifierManager` from `@glimmer/core`:
```ts
import {
modifierCapabilities,
setModifierManager,
} from '@glimmer/core';
```
Also allows users to use simple functions as modifiers:
```js
function scroll(element, position) {
element.scrollY = position;
return () => element.scrollY = 0;
}
```
The function receives the element as the first argument, and any
positional arguments after that. It can return a destructor, which will
be called whenever the modifier is destroyed, or before it is updated
and the modifier is run again.
Also adds a `TrackedObject` implementation for tests, which makes life
for testing a bit easier.
pzuraq
force-pushed
the
feat/modifier-managers
branch
from
793a5d3
to
44e0323
Compare
| export { | ||
| ModifierManager, | ||
| ModifierDefinition, | ||
| capabilities as modifierCapabilities, | ||
| Capabilities as ModifierCapabilities, | ||
| } from './src/managers/modifier'; |
There was a problem hiding this comment.
Thank you for not using export * 👯♂
|
|
||
| if (delegate === undefined) { | ||
| if (DEBUG) { | ||
| assert( |
There was a problem hiding this comment.
Curious, do these get stripped properly on their own or do they have to be within an if (DEBUG) {? Also, if they do have to be in an if (DEBUG) { what happens to the import?
There was a problem hiding this comment.
I believe the import should be tree-shaken, which is why I figured this pattern would be alright for now. We currently only replace DEBUG.
| assert.strictEqual( | ||
| element.innerHTML, | ||
| await settled(), |
There was a problem hiding this comment.
Seems somewhat surprising that settled returns the HTML
There was a problem hiding this comment.
This is the pattern established by the render() test helper. I agree, the more I work with it the more I think we need something a bit better. Ideally we would get QUnit.dom working in this repo, and allow render() to return the result (along with a way to destroy the result).
| const element = document.getElementById('qunit-fixture')!; | ||
| const clickable = element.querySelector(selector)! as HTMLButtonElement; | ||
|
|
||
| clickable.click(); |
There was a problem hiding this comment.
Should be using dispatch event here probably. Seems fine for now, it just reminds me that we really need to genericize the @ember/test-helpers DOM utilities to work without Ember / Glimmer.
| @@ -0,0 +1,58 @@ | |||
| import { consume, tagFor, dirtyTagFor } from '@glimmer/validator'; | |||
There was a problem hiding this comment.
Why not just author this in .ts?
There was a problem hiding this comment.
It is impossible, you can't have new TrackedObject return anything but a TrackedObject if you author it as a TS class, and there's no way to tell TS that this TrackedObject should get the same keys as the object passed to it. @chriskrycho and I noodled on this for a while with tracked-built-ins, and this was the only way I found you could get the expected behavior.
That aside, Proxy itself is very difficult to type correctly. I haven't been able to figure out a way to type the handlers of a Proxy without casting to any multiple times yet. So, I'm not sure TS would help us much with the implementation here.
| propertyDidChange(); | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-empty-function | ||
| let propertyDidChange = function(): void {}; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/explicit-function-return-type | ||
| export function setPropertyDidChange(cb: () => void) { | ||
| propertyDidChange = cb; | ||
| } |
There was a problem hiding this comment.
Can you help me understand this change? Do we not need to be able to customize this anymore?
There was a problem hiding this comment.
This was a mistake I made earlier, propertyDidChange already exists in the getter/setter setup from @glimmer/validator. This meant there were two different propertyDidChange callbacks being fired.
|
I made an issue to track some of the ongoing problems/pain points with the current testing harness: #246 |
Adds modifier managers that match Ember's modifier manager API. They can
be defined by importing
setModifierManagerfrom@glimmer/core:Also allows users to use simple functions as modifiers:
The function receives the element as the first argument, and any
positional arguments after that. It can return a destructor, which will
be called whenever the modifier is destroyed, or before it is updated
and the modifier is run again.
Also adds a
TrackedObjectimplementation for tests, which makes lifefor testing a bit easier.