Outlet reference protocol change for liquid-fire #14149
Conversation
|
☔ The latest upstream changes (presumably #14177) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Due to working on #14177 I actually half way understand this. I think you need to update to incorporate the issues fixed in #14177 (that the outletName itself can be bound and that reference should affect the resulting tag). With these changes, it seems that the Can you add a test to https://github.com/emberjs/ember.js/blob/master/packages/ember-glimmer/tests/integration/outlet-test.js to help us ensure no regressions in this after landing? |
|
Yes, and this PR also needs updates to reflect glimmerjs/glimmer-vm#297. I will work on it. |
This makes the outlet refrence protocol compatible with POJO references so they can round-trip through userspace without breaking (and adds a test to ensure they stay that way). And it updates to glimmer 0.11.4: - the DynamicScope interface now include get/set. - we expose (as intimate API) `-with-dynamic-vars` and `-get-dynamic-var`
|
Updated, tests green, new test added, and I have the liquid-fire test suite fully passing against this.
The I see that some tests are mutating and reusing outletState objects as a shortcut. That is not really testing things the same way they work in apps. The tests in https://github.com/ember-animation/liquid-fire/blob/glimmer2/tests/integration/helpers/liquid-outlet-test.js deliberately copy the outletState each time they set it (look inside the |
Aha! I see. I will try to take a stab at refactoring in a future PR. |
| @@ -290,6 +292,8 @@ export default class Environment extends GlimmerEnvironment { | |||
| return (vm, args) => SimpleHelperReference.create(helper.compute, args); | |||
| } else if (helper.isHelperFactory) { | |||
| return (vm, args) => ClassBasedHelperReference.create(helper, vm, args); | |||
| } else if (helper.glimmerNativeHelper) { | |||
| return helper.glimmerNativeHelper; | |||
There was a problem hiding this comment.
@ef4 I think we can simplify this by making internal helpers just a function with this interface: https://github.com/tildeio/glimmer/blob/master/packages/glimmer-runtime/lib/environment.ts#L239-L241
Then you wouldn't need the isInternalHelper or glimmerNativeHelper flags – which are problematic because a userland helper can pretend to be one of those just by assigning that property on them.
Instead:
let builtInHelper = this.builtInHelpers[name];
if (builtInHelper) {
return builtInHelper;
}
// do userland lookups...
This makes the outlet reference protocol compatible with normal NestedPropertyReferences.
The glimmer2-compatible version of liquid-fire uses dynamically-scoped variable accessors as implemented in glimmerjs/glimmer-vm#294 in order to read and write the outlet state. (This is the value that is named
routeInfoin emberjs/rfcs#95, which exists to eventually make public API for it.)That all works great except for one tiny annoyance in Ember itself: we can't bring an OutletReference into "userspace" and back again. Once we compute a value, we have an untyped POJO that results in a NestedPropertyReference when we pass it back into glimmer.
This change is the simplest thing that works. Alternatively, we should implement a real
RouteInfotype (as described in emberjs/rfcs#95) so that the values can round-trip through userspace and still get converted back into typed references.