Skip to content

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

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

5 participants