[FEATURE ember-string-ishtmlsafe] Added Ember.String.isHtmlSafe.

[workmanw]

RFC: emberjs/rfcs#139

The feature is ready to go. I just need to figure out how to deprecate using:
var safeStr = new Ember.Handlebars.SafeString('<div>Hello</div>');

without breaking safeStr instanceOf Ember.Handlebars.SafeString. If anyone has thoughts on that I'd love to hear them.

TODO:

• Add test for using new Em.Handlebars.SafeString() with Ember.String.isHtmlSafe().
• Remove String.prototype modification.
• Squash

[workmanw]

I think I figured out how to reintroduce this deprecation and maintain the safeStr instanceOf Ember.Handlebars.SafeString functionality using the prototype chain.

It would be awesome if someone could code review my later commit (01fdfa0ae9daafd08ce899a9d6a73935ceebe675) and let me know if that's brilliant or stupid. I'm having a hard time deciding which 😜 .

[rwjblue]

Can you also specify the constructor?

EmberHandlebars.SafeString.prototype = Object.create(SafeString.prototype);
EmberHandlebars.SafeString.prototype.constructor = EmberHandlebars.SafeString;

[rwjblue]

We could also make SafeString a getter, so that just using Ember.Handlebars.SafeString issues the deprecation (even if you didn't invoke. This would help catch foo instanceof Ember.Handlebars.SafeString with a deprecation also...

[workmanw]

Took @rwjblue's advice and converted to using an object getter as the mechanism for deprecation. This is a cleaner solution and covers the deprecation more broadly. i.e. even using str instanceof Ember.Handlebars.SafeString will throw a deprecation (402e7444c6155222c13f5eb3fc8be8b9260028d7).

[rwjblue]

Looks like this needs a rebase. I'd also like to see about moving the test out of ember-htmlbars package (everything we add to ember-htmlbars has to be moved and causes double work at this point). Perhaps add to ember-glimmer, use the newer test harness there, and symlink into ember-htmlbars to ensure the tests run in both environments?

[workmanw]

@rwjblue you mean a squash or just a rebase onto master?

Perhaps add to ember-glimmer, use the newer test harness there, and symlink into ember-htmlbars to ensure the tests run in both environments?

That sounds straightforward. I can give it a shot. Do you have any examples of that off hand?

[workmanw]

@rwjblue I converted the related ember-htmlbars tests over to ember-glimmer tests using the new harness. This could definitely use a review. A few things to call out that I had no idea about:

• Should the safe-string-test.js and string-test.js be in ember-glimmer/tests/compat and ember-glimmer/tests/utils respectfully? These seem to be the first test files with non-integration tests added to ember-glimmer. I put them in compat and utils to maintain consistency w/ ember-htmlbars.
• Additionally, I wasn't sure what test class to use because I couldn't really find other non-integration tests, so I went with TestCase.

Is there someone I should ping for a review who is familiar with the new glimmer test stuff?

I didn't squash these commits to make it easier to review the testing changes. Once I get the 👍 I'll happily squash them down.

[rwjblue]

As far as I can see the RFC (https://github.com/emberjs/rfcs/blob/master/text/0139-isHtmlSafe.md) does not suggest adding this. I think we should avoid adding another thing that modifies String.prototype. Can you remove this block?

[workmanw]

Oh right. I don't know why I did that. Ooops. I'll fix it.

[rwjblue]

Changes look good! Can you also add a test for the deprecated scenario also?

['@test isHtmlSafe should detect SafeString']() {
  let safeString;

  expectDeprecation(() => {
    safeString = new EmberHandlebars.SafeString('<b>test</b>');
  }, 'Ember.Handlebars.SafeString is deprecated in favor of Ember.String.htmlSafe');

  this.assert.ok(isHtmlSafe(safeString));
}

I updated the description with a final countdown checklist...

[workmanw]

@rwjblue Alright, it should be good to go for whenever. Thanks again for your help.

[rwjblue]

Not a blocker for merging, but I'd like to get the deprecation guide updated to mention transition from foo instanceof Ember.Handlebars.SafeString to Ember.String.isHtmlSafe(foo).

[rwjblue]

Thanks for working on this @workmanw! I will add this feature to the agenda for the next core team meeting to see if we can enable in canary (which will eventually be 2.8).

[workmanw]

@rwjblue I'd be happy to get the deprecation guide updated. I'll make sure the polyfill is in working order with this PR and see about linking it also.

[tomdale]

Seems like this should be isHTMLSafe() for consistency? It is checking the isHTML property, not isHtml.

[workmanw]

I went with isHtmlSafe instead of isHTMLSafe to have consistency with it's counterpart, Ember.String.htmlSafe(). I had no strong preference really.

[tomdale]

The convention is to use all lowercase if at the beginning, or all uppercase otherwise. The point is not to always uppercase or always lowercase, but to avoid mixed usage.

[workmanw]

@tomdale Ah, fair enough. I guess I'm not sure how I should have known that. There is nothing in the style guide about case and no one mentioned it during the RFC process.

If there is consensus I can submit a new PR to update the feature and the deprecation guide; as well as change the polyfill.

EDIT: I mean it's not much work at all. Like 10 minutes tops. I don't mind doing it, just wanted to make sure changing it is 👍 since it's a change outside the RFC.

[rwjblue]

Yes, we discussed this at the core team meeting today, and this is the only blocker to enabling the features.

@workmanw - Would you mind submitting an updating PR?

[workmanw]

@rwjblue Sure thing: #13827 .

[workmanw]

Alrighty.

• Code updated.
• Deprecation guide updated.
• Polyfill updated.

Happy friday. 🍻

[tomdale]

@workmanw That convention is not documented AFAIK, and even if you tried to divine it from reading the tea leaves, I believe there are some inconsistencies in the codebase that could lead people astray. I guess that means I should document it somewhere. 😉

[workmanw]

@tomdale No worries. Just trying to make sure I do my own due diligence before submitting PRs.