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.

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

[GLIMMER] Dashed property names #13649

Merged
merged 1 commit into from
Jun 18, 2016

Conversation

chadhietala
Copy link
Contributor

@chadhietala chadhietala commented Jun 11, 2016

Sort of sad that this is even a feature. @krisselden @wycats or @chancancode do you have any suggestions on this, I didn't want to go weaving isBlock all over the place.

Part of #13644

@@ -139,18 +139,6 @@ QUnit.test('Late-registered components can be rendered with ONLY the template re
ok(!helpers['borf-snorlax'], 'Component wasn\'t saved to global helpers hash');
});

test('Component-like invocations are treated as bound paths if neither template nor component are registered on the container', function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duplicated test.

@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2016

This seems good to me...

@chancancode chancancode self-assigned this Jun 15, 2016
@homu
Copy link
Contributor

homu commented Jun 15, 2016

☔ The latest upstream changes (presumably #13658) made this pull request unmergeable. Please resolve the merge conflicts.

@chancancode
Copy link
Member

The syntax refinement in ember is getting pretty confusing, what about something like this?

diff --git a/packages/ember-glimmer/lib/environment.js b/packages/ember-glimmer/lib/environment.js
index ebe1df4..2c769f9 100644
--- a/packages/ember-glimmer/lib/environment.js
+++ b/packages/ember-glimmer/lib/environment.js
@@ -122,6 +122,13 @@ export default class Environment extends GlimmerEnvironment {
   }

   refineStatement(statement) {
+    // 1. resolve any native syntax – if, unless, with, each, and partial
+    let nativeSyntax = super.refineStatement(statement);
+
+    if (nativeSyntax) {
+      return nativeSyntax;
+    }
+
     let {
       isSimple,
       isInline,
@@ -133,37 +140,34 @@ export default class Environment extends GlimmerEnvironment {
       templates
     } = statement;

-    if (key !== 'partial' && isSimple && (isInline || isBlock)) {
+    if (isSimple && (isInline || isBlock)) {
+      // 2. built-in syntaxs
       if (key === 'component') {
         return new DynamicComponentSyntax({ args, templates });
       } else if (key === 'outlet') {
         return new OutletSyntax({ args });
+      }
+
+      // 3. resolve components
+      let internalKey = builtInComponents[key];
+      let definition = null;
+
+      if (internalKey) {
+        definition = this.getComponentDefinition([internalKey]);
       } else if (key.indexOf('-') >= 0) {
-        let definition = this.getComponentDefinition(path);
-
-        if (definition) {
-          wrapClassBindingAttribute(args);
-          wrapClassAttribute(args);
-          return new CurlyComponentSyntax({ args, definition, templates });
-        } else if (isBlock && !this.hasHelper(key)) {
-          assert(`A helper named '${path[0]}' could not be found`, false);
-        }
-      } else {
-        // Check if it's a keyword
-        let mappedKey = builtInComponents[key];
-        if (mappedKey) {
-          let definition = this.getComponentDefinition([mappedKey]);
-          wrapClassBindingAttribute(args);
-          wrapClassAttribute(args);
-          return new CurlyComponentSyntax({ args, definition, templates });
-        }
+        definition = this.getComponentDefinition(path);
+      }
+
+      if (definition) {
+        wrapClassBindingAttribute(args);
+        wrapClassAttribute(args);
+        return new CurlyComponentSyntax({ args, definition, templates });
       }
     }

-    let nativeSyntax = super.refineStatement(statement);
-    assert(`Helpers may not be used in the block form, for example {{#${key}}}{{/${key}}}. Please use a component, or alternatively use the helper in combination with a built-in Ember helper, for example {{#if (${key})}}{{/if}}.`, !nativeSyntax && key && this.hasHelper(key) ? !isBlock : true);
-    assert(`Helpers may not be used in the element form.`, !nativeSyntax && key && this.hasHelper(key) ? !isModifier : true);
-    return nativeSyntax;
+    assert(`Helpers may not be used in the block form, for example {{#${key}}}{{/${key}}}. Please use a component, or alternatively use the helper in combination with a built-in Ember helper, for example {{#if (${key})}}{{/if}}.`, !isBlock || !this.hasHelper(key));
+    assert(`Helpers may not be used in the element form.`, !isModifier || !this.hasHelper(key));
+    assert(`Could not find component named "${key}" (no component or template with that name was found)`, isBlock);
   }

   hasComponentDefinition() {
@@ -179,8 +183,6 @@ export default class Environment extends GlimmerEnvironment {

       if (ComponentClass || layout) {
         definition = this._components[name] = new CurlyComponentDefinition(name, ComponentClass, layout);
-      } else if (!this.hasHelper(name)) {
-        assert(`Glimmer error: Could not find component named "${name}" (no component or template with that name was found)`, !!(ComponentClass || layout));
       }
     }

@chadhietala
Copy link
Contributor Author

@chancancode I tried this locally and all of the late bound layout stuff is going to fail.

@chadhietala chadhietala force-pushed the dashed-properties branch 2 times, most recently from 6a0e26d to ef4c14b Compare June 17, 2016 22:43
@@ -133,37 +170,37 @@ export default class Environment extends GlimmerEnvironment {
templates
} = statement;

if (key !== 'partial' && isSimple && (isInline || isBlock)) {
assert(`You attempted to overwrite the built-in syntax "${key}" which is not allowed. Please rename the helper.`, !(builtInHelpers[key] && this.owner.hasRegistration(`helper:${key}`)));
Copy link
Member

Choose a reason for hiding this comment

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

s/syntax/helper

@chancancode
Copy link
Member

chancancode commented Jun 17, 2016

LGTM when build is green 👍

@krisselden
Copy link
Contributor

There was a problem with installing node earlier, I refreshed Travis. The only failure now is linting. You added a block comment and didn't specify @Private or @public

@chadhietala
Copy link
Contributor Author

Should be good to go now.

@krisselden krisselden merged commit c8824b0 into emberjs:master Jun 18, 2016
@krisselden krisselden deleted the dashed-properties branch June 18, 2016 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants