Assigned attribute values are no longer quoted

[jmerrifield]

I work on a large non-Ember app making extensive use of Emblem templates. We're currently trying to upgrade from 0.4.0 to current but have encountered what seems to be a breaking change that I don't see mentioned in the CHANGELOG.

Given this Emblem template: input type="text" placeholder=placeholder, and the value "multiple words" for placeholder, under 0.4.0 the eventual value for placeholder would be wrapped in quotes:

<!-- Output from Emblem 0.4.0 -->
<input type="text" placeholder="multiple words" />

The newest version, however, results in an unquoted attribute value and broken markup:

<!-- Output from Emblem 0.9.0 -->
<input type="text" placeholder=multiple words>

Is this intentional? Are we now meant to use something like attr="{{ variable }}" instead?

Converting ~14k lines of Emblem to use a different attribute assignment syntax would be prohibitive for us. Would you consider a PR that restored the previous behavior? It looks like adding opening and closing quotes to this line in template-compiler.js would have the desired effect.

[thec0keman]

Emblem 0.5 changed from outputting compiled Ember templates to just outputting raw handlebars at compile time (e.g. with ember-cli). This has a huge advantage as it decouples Emblem from the internal changes Ember has been making with HTMLBars / Glimmer, and allows Emblem just to function as a preprocessor (sort of like a coffeescript addon). It also has an advantage for debugging these kinds of issues: if you can prove that a raw handlebars works as you intend with Ember, then it is rather simple to debug what Emblem is doing or not doing.

For example:

= input type="text" placeholder=placeholder

Compiles to:

{{input type="text" placeholder=placeholder}}

example

The latter is valid handlebars syntax for Ember 2.x, and given a variable 'multiple values', Ember should properly render that string with proper quoting.

So, I'd suggest a next step would be to create an Ember twiddle that showcases your handlebars having the desired effect, and then we can see if Emblem is doing something unexpected.

[quaelin]

Hi @thec0keman, thanks for the reply. I'm working with @jmerrifield on this. Note that we're not using Ember, so an Ember twiddle probably won't be a helpful next step for us.

To clarify the issue we're seeing, here's a really simple example...

Emblem template:

img alt=alt_text

Context:

{
  alt_text: "Multiple words"
}

Emblem 0.4.0 result:

<img alt="Multiple words">

Emblem 0.9.0 output:

<img alt={{alt_text}}>

And then after handlebars applies the context:

<img alt=Multiple words>

Clearly, "Multiple words" at the end there needs to be quoted. It seems like it should be quoted in the Emblem output. That is, Emblem output should look like this, we think:

<img alt="{{alt_text}}">

That would allow our large corpus of old Emblem templates to work with the new version. Would you be receptive to a PR that adds those quotes in template-compiler.js, as Jon suggested?

[thec0keman]

Ah, sorry I misunderstood. That is an interesting problem!

So as far as I can tell, Emblem does not treat HTML attributes differently (other than class and id). So I think to move forward, we'd need to confirm that all HTML attributes that are assigned a bound value can safely be treated as a strings. That seems okay, I can't think of any edge case, although glimmer may throw a wrench in things. (I can't think of a specific reason, just that Emblem does not yet support Glimmer.)

I believe this is the legacy code, and this is the current equivalent. I'm guessing ditching Handlebars.StringNode is what changed the behavior.

I'm not opposed to a PR to fix. It makes sense to continue to support vanilla handlebars, though I know there is a significant portion of Emblem that is very Ember-specific.

[thec0keman]

This seems relevant

[thec0keman]

@jmerrifield @quaelin I released 0.9.2 with this, however quickly noticed it introduces a regression for Ember.

I suspect that this is intended behavior on Ember's part, since it takes care of quoting attributes for you. I'm concerned that this change in Emblem may also introduce further regressions because of Ember's behavior.

I think one alternative would be to move the auto-quoting behind a feature toggle. So, for non-Ember apps, you would do something like:

var emblem = require('emblem').default;

emblem.compile(myEmblemTemplate, {
  useLegacyAttributeQuoting: true
});

Would that solution work in your situation?

[jmerrifield]

That would be fine, we control the compilation stage so can easily supply extra options to emblem.compile.

How would you like to proceed? Are you planning to revert this PR and push a fixed version to NPM? We are out for the holidays so may not be able to work on an option-driven implementation until the new year.

[thec0keman]

:+1: I have a PR that I just opened. I'd like to do a bit of QA, then get it merged as 0.9.3.