Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Outlet reference protocol change for liquid-fire #14149

Merged
merged 1 commit into from
Sep 1, 2016
Merged

Outlet reference protocol change for liquid-fire #14149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"git-repo-info": "^1.1.4",
"git-repo-version": "^0.3.1",
"github": "^0.2.3",
"glimmer-engine": "0.11.3",
"glimmer-engine": "0.11.4",
"glob": "^5.0.13",
"html-differ": "^1.3.4",
"mocha": "^2.4.5",
Expand Down
8 changes: 6 additions & 2 deletions packages/ember-glimmer/lib/environment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import {
Environment as GlimmerEnvironment,
AttributeChangeList,
isSafeString,
compileLayout
compileLayout,
getDynamicVar
} from 'glimmer-runtime';
import Cache from 'ember-metal/cache';
import { assert, warn, runInDebug } from 'ember-metal/debug';
Expand Down Expand Up @@ -108,7 +109,8 @@ export default class Environment extends GlimmerEnvironment {
'-each-in': eachIn,
'-input-type': inputTypeHelper,
'-normalize-class': normalizeClassHelper,
'-html-safe': htmlSafeHelper
'-html-safe': htmlSafeHelper,
'-get-dynamic-var': { glimmerNativeHelper: getDynamicVar }
};
}

Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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...

} else {
throw new Error(`${nameParts} is not a helper`);
}
Expand Down
9 changes: 9 additions & 0 deletions packages/ember-glimmer/lib/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ class DynamicScope {
this.view, this.outletState, this.rootOutletState, this.isTopLevel, this.targetObject
);
}

get(key) {
return this[key];
}

set(key, value) {
this[key] = value;
return value;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the shape for dynamic scope, and also most of the other things on the dynamic scope are private (and more importantly – not a PathRefence<Opaque> as required by the get method on the interface).

Until we have the registration story figured out, we should just throw here (for both get and set) if the key is not === "outletState"

}

const renderers = [];
Expand Down
7 changes: 7 additions & 0 deletions packages/ember-glimmer/lib/syntax.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { OutletSyntax } from './syntax/outlet';
import { MountSyntax } from './syntax/mount';
import { DynamicComponentSyntax } from './syntax/dynamic-component';
import { InputSyntax } from './syntax/input';
import { WithDynamicVarsSyntax } from 'glimmer-runtime';


let syntaxKeys = [];
let syntaxes = [];
Expand All @@ -25,3 +27,8 @@ registerSyntax('outlet', OutletSyntax);
registerSyntax('mount', MountSyntax);
registerSyntax('component', DynamicComponentSyntax);
registerSyntax('input', InputSyntax);
registerSyntax('-with-dynamic-vars', class {
static create(environment, args, templates, symbolTable) {
return new WithDynamicVarsSyntax({ args, templates });
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This could have been a POJO instead of a class)

});
4 changes: 2 additions & 2 deletions packages/ember-glimmer/lib/syntax/outlet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ class OutletComponentReference {


let outletName = outletNameRef.value();
let outletStateRef = parentOutletStateRef.get(outletName);
let outletStateRef = parentOutletStateRef.get('outlets').get(outletName);
let newState = this.lastState = outletStateRef.value();

this.outletStateTag.update(outletStateRef.tag);
Expand Down Expand Up @@ -201,7 +201,7 @@ const TOP_LEVEL_MANAGER = new TopLevelOutletComponentManager();

class OutletComponentManager extends AbstractOutletComponentManager {
create(definition, args, dynamicScope) {
let outletStateReference = dynamicScope.outletState = dynamicScope.outletState.get(definition.outletName);
let outletStateReference = dynamicScope.outletState = dynamicScope.outletState.get('outlets').get(definition.outletName);
Copy link
Member

@chancancode chancancode Sep 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change feels a bit dubious to me – the reason for this change is that you could swap in the outlet state reference for a regular EmberPathReference in your code. However, we are still using the ChildOutletStateReference by default, which doesn't fully behave or duck-type like a regular path reference. For example, it has a get isTopLevel on it, which your EmberPathReference is not going to have. (I think it happens to be unused, but we should just remove it if true.)

If we need to support replacing this with a EmberPathReference, I think we need to actually use that ourselves to guarantee compatibility. At minimum, we need the top-level OutletStateReference to return a EmberPathReference for its get, which would go through the regular paths from there onwards.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we don't have to use an EmberPathReference if we think the ChildOutletStateReference is simpler and faster (and looking at your test reminds me that it could have been any PathReference, in your test it would be a HelperReference). I think the key is that we cannot rely on anything that is not strictly part of the usual PathReference interface – so things like isTopLevel raises a red-flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't really need a separate reference type for outlet states at all. I suspect we would be better off solving this with a better type at the value level, not the reference level.

let outletState = outletStateReference.value();
dynamicScope.targetObject = outletState.render.controller;
return new StateBucket(outletState);
Expand Down
2 changes: 1 addition & 1 deletion packages/ember-glimmer/lib/views/outlet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ class ChildOutletStateReference {
}

value() {
return this.parent.value().outlets[this.key];
return this.parent.value()[this.key];
}

get isTopLevel() {
Expand Down
46 changes: 46 additions & 0 deletions packages/ember-glimmer/tests/integration/outlet-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,4 +278,50 @@ moduleFor('outlet view', class extends RenderingTest {

this.assertText('HIBAR');
}

['@test outletState can pass through user code (liquid-fire initimate API) ']() {
this.registerTemplate('outer', 'A{{#-with-dynamic-vars outletState=(identity (-get-dynamic-var "outletState"))}}B{{outlet}}D{{/-with-dynamic-vars}}E');
this.registerTemplate('inner', 'C');

// This looks like it doesn't do anything, but its presence
// guarantees that the outletState gets converted from a reference
// to a value and then back to a reference. That is what we're
// testing here.
this.registerHelper('identity', ([a]) => a);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this do something instead of pass-through? This allows you to test updating behavior (see below)


let outletState = {
render: {
owner: this.owner,
into: undefined,
outlet: 'main',
name: 'outer',
controller: {},
ViewClass: undefined,
template: this.owner.lookup('template:outer')
},
outlets: {
main: {
render: {
owner: this.owner,
into: undefined,
outlet: 'main',
name: 'inner',
controller: {},
ViewClass: undefined,
template: this.owner.lookup('template:inner')
},
outlets: Object.create(null)
}
}
};

this.runTask(() => this.component.setOutletState({ render: {}, outlets: { main: outletState } }) );

runAppend(this.component);

this.assertText('ABCDE');

this.assertStableRerender();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, we want to make sure the updating paths work and so we follow the I-N-U-R pattern for new tests.

Maybe you can get your helper to override the template based on a dynamic property, which would allow you to change it to a new template then replace it with the original. (Or if you could get it to do something similar to what LF is ultimately doing, then even better.)

}

});