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 2] invalid inlineclassNameBindings inlink-to yields an exception #13955

Closed
GavinJoyce opened this issue Jul 29, 2016 · 7 comments
Closed
Labels

Comments

@GavinJoyce
Copy link
Member

GavinJoyce commented Jul 29, 2016

The invalid classNameBindings below behaves inconsistently in htmlbars and glimmer (see https://github.com/GavinJoyce/glammer/blob/master/app/templates/issue1.hbs).

{{#link-to 'index' classNameBindings="someProperty"}}

htmlbars:

no warning, the follow is rendered:

<a id="ember369" href="/" class="  ember-view">Click me to go to the index</a>

glimmer:

an exception is thrown when rendering:

Cannot read property '0' of null (in hasHelper)

screen shot 2016-07-29 at 14 21 46

Some questions:

  • Should glimmer 2 behave the same as htmlbars?
  • Should we issue a warning in development mode?
@rwjblue
Copy link
Member

rwjblue commented Jul 29, 2016

We should issue a helpful assertion. Ideally in both engines.

@GavinJoyce
Copy link
Member Author

GavinJoyce commented Jul 29, 2016

Thanks, I can take this on

@GavinJoyce
Copy link
Member Author

GavinJoyce commented Aug 4, 2016

TL;DR: it looks like we're generating an incorrect spec


For the template:

{{foo-bar classNameBindings="someInitiallyTrueProperty isBig:big:small"}}

Here's the processed AST (with loc info removed):

{
  "type": "Program",
  "body": [
    {
      "type": "MustacheStatement",
      "path": {
        "type": "PathExpression",
        "data": false,
        "original": "foo-bar",
        "parts": [
          "foo-bar"
        ]
      },
      "params": [],
      "hash": {
        "type": "Hash",
        "pairs": [
          {
            "type": "HashPair",
            "key": "class",
            "value": {
              "type": "SubExpression",
              "path": {
                "type": "PathExpression",
                "original": "concat",
                "parts": [
                  "concat"
                ],
                "data": false
              },
              "params": [
                {
                  "type": "SubExpression",
                  "path": {
                    "type": "PathExpression",
                    "original": "if",
                    "parts": [
                      "if"
                    ],
                    "data": false
                  },
                  "params": [
                    {
                      "type": "PathExpression",
                      "original": "someInitiallyTrueProperty",
                      "parts": [
                        "someInitiallyTrueProperty"
                      ],
                      "data": false
                    },
                    {
                      "type": "SubExpression",
                      "path": {
                        "type": "StringLiteral",
                        "value": "-normalize-class",
                        "original": "-normalize-class"
                      },
                      "params": [
                        {
                          "type": "StringLiteral",
                          "value": "someInitiallyTrueProperty",
                          "original": "someInitiallyTrueProperty"
                        },
                        {
                          "type": "PathExpression",
                          "original": "someInitiallyTrueProperty",
                          "parts": [
                            "someInitiallyTrueProperty"
                          ],
                          "data": false
                        }
                      ],
                      "hash": {
                        "type": "Hash",
                        "pairs": []
                      }
                    }
                  ],
                  "hash": {
                    "type": "Hash",
                    "pairs": []
                  }
                },
                {
                  "type": "StringLiteral",
                  "value": " ",
                  "original": " "
                },
                {
                  "type": "SubExpression",
                  "path": {
                    "type": "PathExpression",
                    "original": "if",
                    "parts": [
                      "if"
                    ],
                    "data": false
                  },
                  "params": [
                    {
                      "type": "PathExpression",
                      "original": "isBig",
                      "parts": [
                        "isBig"
                      ],
                      "data": false
                    },
                    {
                      "type": "StringLiteral",
                      "value": "big",
                      "original": "big"
                    },
                    {
                      "type": "StringLiteral",
                      "value": "small",
                      "original": "small"
                    }
                  ],
                  "hash": {
                    "type": "Hash",
                    "pairs": []
                  }
                },
                {
                  "type": "StringLiteral",
                  "value": " ",
                  "original": " "
                }
              ],
              "hash": {
                "type": "Hash",
                "pairs": []
              }
            }
          }
        ]
      },
      "escaped": true
    }
  ],
  "blockParams": []
}

This then produces the following spec which looks incorrect (there's nothing about -normalize-class in it):

{
  "statements": [
    [
      "append",
      [
        "helper",
        [
          "foo-bar"
        ],
        null,
        {
          "class": [
            "helper",
            [
              "concat"
            ],
            [
              [
                "helper",
                [
                  "if"
                ],
                [
                  [
                    "get",
                    [
                      "someInitiallyTrueProperty"
                    ]
                  ],
                  [
                    "helper",
                    null,
                    [
                      "someInitiallyTrueProperty",
                      [
                        "get",
                        [
                          "someInitiallyTrueProperty"
                        ]
                      ]
                    ],
                    null
                  ]
                ],
                null
              ],
              " ",
              [
                "helper",
                [
                  "if"
                ],
                [
                  [
                    "get",
                    [
                      "isBig"
                    ]
                  ],
                  "big",
                  "small"
                ],
                null
              ],
              " "
            ],
            null
          ]
        }
      ],
      false
    ]
  ],
  "locals": [],
  "named": [],
  "yields": [],
  "blocks": [],
  "meta": {
    "moduleName": "-top-level"
  }
}

Here are the equivalent htmlbars statements:

statements: [
  ["inline","foo-bar",[],["class",["subexpr","concat",[["subexpr","if",[["get","someInitiallyTrueProperty",[],0,0,0,0],["subexpr","-normalize-class",["someInitiallyTrueProperty",["get","someInitiallyTrueProperty",[],0,0,0,0]],[],[],0,0]],[],[],0,0]," ",["subexpr","if",[["get","isBig",[],0,0,0,0],"big","small"],[],[],0,0]," "],[],[],0,0]],["loc",[null,[1,0],[1,73]]],0,0]
]

@GavinJoyce
Copy link
Member Author

GavinJoyce commented Aug 4, 2016

Failing test: #14019
Related PR where the glimmer -normalize-class helper was added: #13724

@GavinJoyce
Copy link
Member Author

@chadhietala perhaps you might be able to add some insight here?

@chadhietala
Copy link
Contributor

Let me take a look 🙇

@GavinJoyce
Copy link
Member Author

This was fixed in #14028.

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 a pull request may close this issue.

4 participants