-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Adds type annotations for ember-glimmer macro implementations #15862
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,11 @@ import { ConstReference, isConst } from '@glimmer/reference'; | |
import { | ||
Arguments, | ||
VM, | ||
OpcodeBuilderDSL, | ||
ComponentArgs, | ||
} from '@glimmer/runtime'; | ||
import * as WireFormat from '@glimmer/wire-format'; | ||
import { Option } from '@glimmer/util'; | ||
import { assert } from 'ember-debug'; | ||
import { | ||
NON_SINGLETON_RENDER_MANAGER, | ||
|
@@ -17,7 +21,7 @@ import Environment from '../environment'; | |
import { OwnedTemplate } from '../template'; | ||
import { hashToArgs } from './utils'; | ||
|
||
function makeComponentDefinition(vm: VM, args: Arguments) { | ||
function makeComponentDefinition(vm: VM, args: Arguments): ConstReference<RenderDefinition> { | ||
let env = vm.env as Environment; | ||
let nameRef = args.positional.at(0); | ||
|
||
|
@@ -128,12 +132,17 @@ function makeComponentDefinition(vm: VM, args: Arguments) { | |
@public | ||
@deprecated Use a component instead | ||
*/ | ||
export function renderMacro(_name: string, params: any[], hash: any[], builder: any) { | ||
export function renderMacro(_name: string, params: Option<WireFormat.Core.Params>, hash: WireFormat.Core.Hash, builder: OpcodeBuilderDSL): boolean { | ||
if (!params) { | ||
params = []; | ||
} | ||
let definitionArgs = [params.slice(0), hash, null, null]; | ||
let args = [params.slice(1), hashToArgs(hash), null, null]; | ||
builder.component.dynamic(definitionArgs, makeComponentDefinition, args); | ||
let definitionArgs: ComponentArgs = [params.slice(0), hash, null, null]; | ||
let args: ComponentArgs = [params.slice(1), hashToArgs(hash), null, null]; | ||
|
||
// We have to coerce makeComponentDefinition into an `any` here because | ||
// Glimmer VM is typing this too restrictively as a PathReference when it | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you cross link to the issue in glimmer-vm that you will be opening to track removal of the coercion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This API no longer exists in Glimmer VM, so I don't think I will be opening that issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tomdale - So a future update of glimmer-vm will remove this comment (and what it is doing)? Can we link to anything that will help us understand this when we come back to this code in 6 months? I fear without any pointers to additional things to dig into, this will become yet another murder mystery for our future selves to solve. 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome, thank you (sorry for the run around). |
||
// should just be a Reference (paths are never looked up on | ||
// ComponentDefinitions. | ||
builder.component.dynamic(definitionArgs, makeComponentDefinition as any, args); | ||
return true; | ||
} |
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 believe this is stripped in glimmer-vm, but I don’t believe that is true here.
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 had that question too, @chadhietala probably knows that stuff the most intimately.