Skip to content
This repository has been archived by the owner on Apr 18, 2023. It is now read-only.

Replaced usage of new Ember.Handlebars.SafeString with Ember.String.htmlSafe #363

Closed
wants to merge 1 commit into from

Conversation

workmanw
Copy link
Contributor

Using SafeString in this way has been deprecated. See: http://emberjs.com/deprecations/v2.x/#toc_use-ember-string-htmlsafe-over-ember-handlebars-safestring

Ember.String.htmlSafe has been around since Ember 1.0.0 so I don't see any concerns about backwards compatibility.

@workmanw
Copy link
Contributor Author

Obviously there is a failing test here. The TL;DR; is that we can no longer test with instanceof Ember.Handlebars.SafeString. I've submitted an issue to Ember: emberjs/ember.js#13354 . Pending the outcome of that, I'll circle back around fix this.

@jamesarosen
Copy link
Owner

Build now failing on beta: https://travis-ci.org/jamesarosen/ember-i18n/jobs/126707715

@workmanw
Copy link
Contributor Author

workmanw commented May 3, 2016

@jamesarosen Sorry for the delay. I circled back around this morning on the emberjs issue. There is now a PR open to remove the deprecation: emberjs/ember.js#13442 . When that is merged and a new beta release is cut, the tests should pass once again. It seems the plan is still to switch to Ember.String.htmlSafe, so once the next build of emberjs-beta drops, I would advocate we merge this.

@devotox
Copy link

devotox commented May 8, 2016

The new beta has dropped should this be retested?

@workmanw
Copy link
Contributor Author

workmanw commented May 9, 2016

I'm away from my computer until tonight. Once I get home I'll force push a
new commit to kick-off a new set of test runs. Sorry for the delay!
On May 8, 2016 4:47 PM, "Devonte" notifications@github.com wrote:

The new beta has dropped should this be retested?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#363 (comment)

…g.htmlSafe`. Using `SafeString` in this way has been deprecated.
@workmanw
Copy link
Contributor Author

@devotox @jamesarosen Looks like the tests pass now. I think this should still be merged even though using new Ember.Handlebars.SafeString is no longer deprecated.

@jamesarosen
Copy link
Owner

What about the new is-safe implementation that we use in the tests?

@workmanw
Copy link
Contributor Author

@jamesarosen That's still in the RFC phase. Unfortunately there has been little activity there. It works now because htmlSafe will still return an instanceof Ember.Handlebars.SafeString.

Maybe the better path would be to hold on this until that RFC goes through and this feature lands.

@jamesarosen
Copy link
Owner

Maybe the better path would be to hold on this until that RFC goes through and this feature lands.

I'm happy either way. Your call.

@workmanw
Copy link
Contributor Author

@jamesarosen Honestly, after thinking about it, I do think we should hold. This merge is not prudent and once isHtmlSafe lands, will have to commit again. I'm going to close this and I'll take it back up in the future.

@workmanw workmanw closed this May 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants