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

workmanw · 2016-06-14T12:37:44Z

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 · 2016-06-14T13:47:51Z

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 · 2016-06-14T16:54:30Z

packages/ember-htmlbars/lib/compat.js

+
+  SafeString.apply(this, arguments);
+};
+EmberHandlebars.SafeString.prototype = Object.create(SafeString.prototype);


Can you also specify the constructor?

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

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 · 2016-06-14T18:02:54Z

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 · 2016-06-16T01:31:49Z

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 · 2016-06-16T02:15:32Z

@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 · 2016-06-19T15:27:27Z

@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 · 2016-06-19T15:44:57Z

packages/ember-htmlbars/lib/utils/string.js

+if (isEnabled('ember-string-ishtmlsafe')) {
+  EmberStringUtils.isHtmlSafe = isHtmlSafe;
+  if (ENV.EXTEND_PROTOTYPES.String) {
+    String.prototype.isHtmlSafe = function() {


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?

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

rwjblue · 2016-06-19T15:46:55Z

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

…rjs/rfcs#139). Reintroduced deprecation of `new Ember.Handlebars.SafeString()`.

workmanw · 2016-06-19T17:16:56Z

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

rwjblue · 2016-06-19T18:59:55Z

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 · 2016-06-19T19:01:31Z

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 · 2016-06-19T20:09:06Z

@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 · 2016-07-15T18:22:53Z

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

workmanw · 2016-07-15T20:08:29Z

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

tomdale · 2016-07-15T21:19:36Z

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 · 2016-07-15T22:53:29Z

@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 · 2016-07-16T01:25:33Z

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 · 2016-07-16T01:53:10Z

@rwjblue Sure thing: #13827 .

workmanw · 2016-07-16T02:48:10Z

Alrighty.

Code updated.
Deprecation guide updated.
Polyfill updated.

Happy friday. 🍻

tomdale · 2016-07-19T15:09:35Z

@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 · 2016-07-19T18:33:16Z

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

workmanw force-pushed the is-html-safe branch 3 times, most recently from 85f3822 to e7dd499 Compare June 14, 2016 13:44

workmanw force-pushed the is-html-safe branch from e7dd499 to 01fdfa0 Compare June 14, 2016 13:52

workmanw changed the title ~~[WIP] [FEATURE ember-string-ishtmlsafe] Added Ember.String.isHtmlSafe.~~ [FEATURE ember-string-ishtmlsafe] Added Ember.String.isHtmlSafe. Jun 14, 2016

rwjblue reviewed Jun 14, 2016
View reviewed changes

workmanw force-pushed the is-html-safe branch from 01fdfa0 to 402e744 Compare June 14, 2016 18:00

workmanw force-pushed the is-html-safe branch from 402e744 to e61b440 Compare June 19, 2016 13:29

rwjblue reviewed Jun 19, 2016
View reviewed changes

workmanw force-pushed the is-html-safe branch from a5d622c to c920ae5 Compare June 19, 2016 16:30

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

43b2238

…rjs/rfcs#139). Reintroduced deprecation of `new Ember.Handlebars.SafeString()`.

workmanw force-pushed the is-html-safe branch from c920ae5 to 43b2238 Compare June 19, 2016 16:33

rwjblue merged commit 1083dac into emberjs:master Jun 19, 2016

homu mentioned this pull request Jun 19, 2016

[FEATURE ember-metal-weakmap] Add a WeakMap polyfill #13688

Merged

workmanw mentioned this pull request Jul 16, 2016

[FEATURE ember-string-ishtmlsafe] Changed isHtmlSafe to isHTMLSafe for consistency. #13827

Merged

workmanw mentioned this pull request Jul 16, 2016

Updated the deprecation for ember-htmlbars.ember-handlebars-safestring. emberjs/website#2645

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

workmanw commented Jun 14, 2016 •

edited

Loading

workmanw commented Jun 14, 2016 •

edited

Loading

rwjblue Jun 14, 2016

rwjblue Jun 14, 2016

workmanw commented Jun 14, 2016 •

edited

Loading

rwjblue commented Jun 16, 2016

workmanw commented Jun 16, 2016

workmanw commented Jun 19, 2016

rwjblue Jun 19, 2016

workmanw Jun 19, 2016

rwjblue commented Jun 19, 2016

workmanw commented Jun 19, 2016

rwjblue commented Jun 19, 2016

rwjblue commented Jun 19, 2016

workmanw commented Jun 19, 2016

tomdale commented Jul 15, 2016

workmanw commented Jul 15, 2016

tomdale commented Jul 15, 2016 •

edited

Loading

workmanw commented Jul 15, 2016 •

edited

Loading

rwjblue commented Jul 16, 2016 •

edited

Loading

workmanw commented Jul 16, 2016

workmanw commented Jul 16, 2016

tomdale commented Jul 19, 2016

workmanw commented Jul 19, 2016

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

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

Conversation

workmanw commented Jun 14, 2016 • edited Loading

workmanw commented Jun 14, 2016 • edited Loading

rwjblue Jun 14, 2016

Choose a reason for hiding this comment

rwjblue Jun 14, 2016

Choose a reason for hiding this comment

workmanw commented Jun 14, 2016 • edited Loading

rwjblue commented Jun 16, 2016

workmanw commented Jun 16, 2016

workmanw commented Jun 19, 2016

rwjblue Jun 19, 2016

Choose a reason for hiding this comment

workmanw Jun 19, 2016

Choose a reason for hiding this comment

rwjblue commented Jun 19, 2016

workmanw commented Jun 19, 2016

rwjblue commented Jun 19, 2016

rwjblue commented Jun 19, 2016

workmanw commented Jun 19, 2016

tomdale commented Jul 15, 2016

workmanw commented Jul 15, 2016

tomdale commented Jul 15, 2016 • edited Loading

workmanw commented Jul 15, 2016 • edited Loading

rwjblue commented Jul 16, 2016 • edited Loading

workmanw commented Jul 16, 2016

workmanw commented Jul 16, 2016

tomdale commented Jul 19, 2016

workmanw commented Jul 19, 2016

workmanw commented Jun 14, 2016 •

edited

Loading

workmanw commented Jun 14, 2016 •

edited

Loading

workmanw commented Jun 14, 2016 •

edited

Loading

tomdale commented Jul 15, 2016 •

edited

Loading

workmanw commented Jul 15, 2016 •

edited

Loading

rwjblue commented Jul 16, 2016 •

edited

Loading