-
Notifications
You must be signed in to change notification settings - Fork 190
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
[Feat]: element keyword #1313
base: main
Are you sure you want to change the base?
[Feat]: element keyword #1313
Conversation
fa8cfbd
to
0a8147a
Compare
@snewcomer you still motivated to finish this? (how close is it? should we find help?) |
@NullVoxPopuli Would love to! I just need some confidence that the work I get done will actually be reviewed and merged though. e.g. #1277 |
0a8147a
to
a7a4248
Compare
const AComponent = defineComponent({}, '{{element "div" 1}}'); | ||
this.renderComponent(AComponent); | ||
}, syntaxErrorFor('(element) only takes one positional argument - the element tag name', '{{element "div" 1}}', 'an unknown module', 1, 0)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets have one for named args too, since I think we want to reserve the space for that to mean attributes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also {{element true}}
etc errors
this.renderComponent(AComponent); | ||
}, syntaxErrorFor('(element) only takes one positional argument - the element tag name', '{{element "div" 1}}', 'an unknown module', 1, 0)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add some tests for:
<Tag>
Some dynamic content here.
When "Tag" changes, the content here get torn down and re-rendered.
You can probably assert that with a helper that does `count++` or something similar.
</Tag>
According to https://github.com/emberjs/rfcs/pull/620/files#r418378655
{{#let (element "") as |Tag|}}
<Tag>Hello</Tag>
{{/let}}
behaves like
Hello (no wrapping tag)
According to https://github.com/emberjs/rfcs/pull/620/files#r418379071
{{#let (element @tagName) as |Tag|}}
<Tag>Hello</Tag>
{{/let}}
Apparently, when @tagName changes between "div" and null/undefined, the above behaves like:
{{#if @tagName}}
<div>Hello</div>
{{/if}}
let lastValue: Maybe<Dict> | string, curriedDefinition: object | string | null; | ||
|
||
return createComputeRef(() => { | ||
let value = valueForRef(tagName) as string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add runtime assertions here, just the ones you have in the compiler is not sufficient because the compiler can't check (element @foo)
that @foo
is a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Therefore also tests that makes this dynamic instead of a literal)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, according to the RFC this should at least be string | null | undefined
and I think we should normalize the null/undefined here with something along the lines of
let _value = valueForRef(tagName);
assert(...);
let value = _value ?? null /* as string | null – the assertions should give you this cast "for free" */;
|
||
if (value === lastValue) { | ||
return curriedDefinition; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code will be easier to write if you do
let lastValue = null;
let component = null;
return createComputeRef(() => {
// ...
if (value === lastValue) {
return component;
}
lastValue = value;
component = ...;
return component;
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think that we shouldn't need the "curry" stuff here anymore. If I understand correctly, things are supposed to Just Work™ when you do <Tag>
where Tag
is a component. So I don't think we need to go out of our way to "curry nothing" here.
On a related note I think we also try to hide what the return type is since it can leak into user land JS code an ultimately it will be depended on by app code somehow (in this case the DynamicElementComponentDefinition
class and the fact that it has a tagName
property on it will accidentally become public api).
So we can do this dance (or there may be a similar pattern elsewhere in glimmer-vm already?):
// possibly, we can do less work between DEBUG and production, but I think it doesn't really matter
// since this shouldn't be a super common feature?
// not exported, not public
class DynamicElement {
constructor(readonly tagName: string) {}
}
const DYNAMIC_ELEMENTS = new WeakMap<object, DynamicElement>();
function makeDynamicElement(tagName: string | null): object | null {
if (tagName === null) {
return null;
}
let component = new DynamicElement(tagName);
let opaqueValue = Object.freeze({
// just so you get something nice to look at when logging to the console
toString() {
return `(element "${tagName}")`;
}
});
setInternalComponentManager(DYNAMIC_ELEMENT_MANAGER, opaqueValue);
DYNAMIC_ELEMENTS.set(opaqueValue, component);
return opaqueValue;
}
It seems like this PR implemented the part where |
WIP
Ref polyfill -
https://github.com/tildeio/ember-element-helper
TODO