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

Fix Handlebars.SafeString deprecation message in ember 2.8.x #26

Merged
merged 1 commit into from
Oct 11, 2016

Conversation

tehmaestro
Copy link
Contributor

@@ -16,7 +16,7 @@ const {
} = Ember;

function isSafeString(input) {
return (typeof isHTMLSafe === 'function' && isHTMLSafe(input)) || (input instanceof Handlebars.SafeString);
return (typeof isHTMLSafe === 'function' && isHTMLSafe(input)) || (input && typeof input.toHTML === 'function');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toHTML doesn't look to be safe
emberjs/ember.js#13318 (comment)

Copy link
Contributor Author

@tehmaestro tehmaestro Oct 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I see that the code is being used in the isHTMLSafe function itself inside ember https://github.com/emberjs/ember.js/blob/aeeddf7c00b42a340494465252481b5414d87009/packages/ember-glimmer/lib/utils/string.js

So the changes in my commit are actually duplicated. I guess the best way would be to just use the isHTMLSafe function for ember versions that support it, and only use instanceof SafeString in older ember versions that will not throw a deprecation message.

Maybe something like this:

function isSafeString(input) {
    if (typeof isHTMLSafe === 'function') {
         return isHTMLSafe(input);
    }
   return input instanceof Handlebars.SafeString;
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me

@tehmaestro tehmaestro reopened this Oct 10, 2016
@jasonmit
Copy link
Owner

LGTM, thanks @tehmaestro

Tests failing for non-related reasons so I'll merge and version

@jasonmit jasonmit merged commit 4d6c8b5 into jasonmit:master Oct 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants