Deprecation of Handlebars.SafeString prevents clear checks if something is a SafeString

[dustyjewett]

With the deprecation of Handlebars.SafeString, the constructor chain of SafeString doesn't include Handlebars.SafeString, making instanceof checks fail.

See Twiddle: https://ember-twiddle.com/32e5e3818cd66aa224f5a422fdafd83a?openFiles=controllers.application.js%2C

For context, the latest change in the chain, fixing the deprecation:
https://github.com/emberjs/ember.js/pull/13226/files

[rwjblue]

Can you help me understand why you need to check if a given string is a SafeString?

[dustyjewett]

We were using this in a test, verifying that our helper returned a SafeString.

[topaxi]

I have some library code that uses this to emit a warning when an unsafe string was passed.

https://github.com/topaxi/ember-custom-css-properties/blob/master/addon/mixins/custom-css-properties.js#L67

[rwjblue]

@dustyjewett - Wouldn't it be better to just use the helper, and ensure its return value was displayed as HTML?

moduleForComponent('asdf', { integration: true });

test('renders html', function(assert) {
  this.render(hbs`{{some-helper}}`);

  assert.equal(this.$('.some-selector-for-html-values-from-helper').length, 1, 'found html from helper');
});

[rwjblue]

@topaxi - We generally use a toHTML function in the HTMLBars internals to determine if a thing is a safe string. This may change with Glimmer (I believe a symbol is used in Glimmer), but that is still TBD.

[dustyjewett]

@rwjblue We certainly test that our end result is as-expected, but because SafeString has an explicit impact in how helpers are rendered into the DOM, we want to check that as well.

To get around this issue, we verify that the return object is NOT a string, but that the .toString() is as-expected. But that is just testing known ways that things might go wrong, we can't check the actual, expected behavior.

(also, many of our objects have toHTML methods, so checking that won't ensure that we didn't accidentally just return one of our other classes)

[pixelhandler]

@dustyjewett sounds like the deprecation has a side-effect if you expect to check the instance, new Ember.Handlebars.SafeString('string') instanceof Ember.Handlebars.SafeString;

I'm curious how hard would it be in your app to make the suggested change to Ember.String.htmlSafe. Also, would using that allow you to use instanceof again?

(I'm looking for a workaround, my best guess is that the deprecation may not change for this use case.)

[rwjblue]

I think we may need to expose a utility function. Something like isHTMLSafe(something)...

[dustyjewett]

@pixelhandler: I believe we already changed to htmlSafe(), but it still doesn't give us a way to confirm that an object is a SafeString.

I'm fine with duck-typing... adding isSafeString:true to the object(s) would be fine with me... just some way to check.

Ember.String.htmlSafe('string') instanceof Ember.Handlebars.SafeString is also false.

[rwjblue]

Ya, I was saying we might need to provide a method (not adding a prop to the objects). Adding a property to the object, doesn't prevent against issues where you may be JSON.parseing an untrusted object (ala CVE-2015-7565).

[zackthehuman]

If you used a "special" property where the value was a reference to a known function it shouldn't be JSON.parse- or JSON.stringify-able. I think this needs to be addressed in Glimmer as well since Symbols may not work out for the time being.

[mixonic]

If there is a use-case for detecting safe strings, I expect it would make a good feature addition. Ember.String.isHtmlSafe(someString) or something.

[rwjblue]

@mixonic - Yes, precisely.

[workmanw]

Copy over from #13354:

In our app we have a few occasions where we do if (str instanceof Ember.Handlebars.SafeString) when handling string values, this capability is broken with the recent deprecation of new Ember.Handlebars.SafeString("") because of the way in which this deprecation was introduced.

See: 0e95d7c

The problem is that the function SafeString is no longer publically accessible. What was Ember.Handlebars.SafeString is now a wrapper function around htmlSafe.

It isn't just our app either. While submitting a PR to Ember-i18n I discovered their tests are broken because they too do not have access to this function. See: https://github.com/jamesarosen/ember-i18n/blob/master/tests/unit/utils/i18n/default-compiler-test.js#L56-L59 Addtionally, if you search for instanceof Ember.Handlebars.SafeString you'll find quite a few cases of people doing the same: https://github.com/search?q=instanceof+Ember.Handlebars.SafeString&type=Code&utf8=%E2%9C%93

While I really like @mixonic's suggestion for isHtmlSafe, I think there is definitely a risk of breaking backwards compatibility. At the very least this should be noted as a breaking change on the blog and the deprecation guide.

EDIT: I have some bandwidth at the moment, I'd be happy to submit a PR for isHtmlSafe if that has the green light.

[rwjblue]

I think the Ember.String.isHtmlSafe utility function would need to go through RFC process (as a public API addition), but I suspect it would be a fairly quick process (as the need seems fairly clear now). It would also be possible to polyfill isHtmlSafe for older Ember versions (which can be used by addons to avoid deprecation for both older and newer versions).

I am a bit torn here, as I really do think we should deprecate Ember.Handlebars.SafeString (it is a very trolling API), but it seems that the ability to detect if a given string is a SafeString or not was a feature of the system that I didn't account for in the initial deprecation. I believe the correct course of action would be to revert the deprecation and try to push it through along with the new API for detection.

[rwjblue]

@workmanw - Would you mind working up the RFC?

[workmanw]

@rwjblue I was just responding with: "I'll get the ball rolling on the RFC PR and build a polyfill." :)

[workmanw]

@rwjblue So this is still an issue with v2.6.0-beta3. Do you still think we should revert the deprecation? If so, I can submit a PR that rolls back #13204.

[rwjblue]

Submitted #13442 to remove.

[workmanw]

@rwjblue Probably okay to close now that is Ember.String.isHtmlSafe is merged?

Polyfill solution is available here: https://github.com/workmanw/ember-string-ishtmlsafe-polyfill

[rwjblue]

Thanks!