-
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
#139 + Upsert refactor #142
Conversation
This creates a new type of Append opcode that handles the value when it is a SafeString and also when it is a normal string. For SafeString it enters into "html" mode, and if the input changes into a normal string it switches into "text" mode. It can switch freely between the two modes and handles updates accordingly. The opcode also supports const expressions. New tests have been added which test the switching behavior of the opcode.
Support for htmlSafe/SafeString
|
||
type PositionalArguments = Opaque[]; | ||
type KeywordArguments = Dict<Opaque>; | ||
|
||
export interface Helper { | ||
(args: EvaluatedArgs): PathReference<Opaque>; |
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.
@zackthehuman is this change intentional? I believe we actually want helpers to return PathReference
s (see emberjs/ember.js#13173 (comment))
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.
Yes, otherwise there doesn't seem to be a way to return a ConstReference from a Helper since they aren't PathReferences. Is there a better way to resolve?
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 wanted a helper that returned a ConstReference
so we could write a test for const SafeStrings
and Node
s.
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.
Yep, @krisselden told me about it. There is a few subclasses of ConstReference
that implements PathReference
, I'll fix it!
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 was surprised that this change didn't cause any type errors, and I just figured out why. In https://github.com/tildeio/glimmer/blob/master/packages/glimmer-runtime/lib/compiled/expressions/helper.ts#L22, we used Function.prototype.call
which is typed to be call(thisArg: any, ...args: any[]): any
, so the return value became any
and the type system happily let it go through, even though the method is supposed to return a PathReference<Opaque>
😞
Fixing that as well
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.
@zackthehuman @krisselden fixed in 8001311 👍
f60604c
to
723b68e
Compare
Move all the upset code into objects: each object only need to know how to update within the same type (e.g. html -> html, Node -> Node). If they fail to update because the type has changed, the parent can simply clear the rendered content and make a new upsert object of the appropriate type. Also fixed CompiledHelpers claiming to correctly return a `PathReference<Opaque>`.
Helpers are required to return `PathReference`s`, because their return values can be later used in lookup positions, e.g. ``` {{#with (my-helper) as |foo|}} {{foo.bar}} <--- referenceFromMyHelper.get('bar') {{/with}} ``` See emberjs/ember.js#13173 (comment) for more details. We were previously using `Function.prototype.call` to invoke helpers, which is typed to be `call(thisArg: any, args: any[]): any` which caused us to not get a TypeError from the change (because the return value is `any`). This commit fixed that as well.
Merged in the latest (4148235) changes in #139 with the upsert refactor:
Move all the upset code into objects in a new file: each object only need to know how to update within the same type (e.g. html -> html, Node -> Node). If they fail to update because the type has changed, the parent can simply clear the rendered content and make a new upsert object of the appropriate type.
Also fixed CompiledHelpers claiming to correctly return a
PathReference<Opaque>
./cc @zackthehuman @krisselden