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

[BUGFIX release] Revert deprecation of htmlSafe and isHTMLSafe #19396

Merged
merged 2 commits into from
Feb 11, 2021

Conversation

locks
Copy link
Contributor

@locks locks commented Feb 11, 2021

Starts addressing #19393.

@locks locks requested review from rwjblue and kategengler February 11, 2021 00:28
Comment on lines 674 to 689
Object.defineProperty(Ember.String, 'htmlSafe', {
enumerable: true,
configurable: true,
get() {
deprecateImportFromString('htmlSafe');
return htmlSafe;
},
});
Object.defineProperty(Ember.String, 'isHTMLSafe', {
enumerable: true,
configurable: true,
get() {
deprecateImportFromString('isHTMLSafe');
return isHTMLSafe;
},
});
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, don't we still need these properties? Were they already there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved them up with the rest of of the Ember.String methods: https://github.com/emberjs/ember.js/pull/19396/files#diff-2f99d92a23bae60fccaee9fcfeda8b77ca73723657662cfadfa4d82751045e14R453.
Since we're not calling the deprecation method, I thought they could all be assigned together.

Copy link
Member

Choose a reason for hiding this comment

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

D'oh! Sorry about that, I totally missed it in the diff.

Since we're not calling the deprecation method, I thought they could all be assigned together.

I think that maybe it would be better to keep the infra around for the deprecation, and change the deprecateImportFromString to pass true to deprecate (with a nice comment explaining that it needs to be changed back to false when we have addressed that issue).

Comment on lines 674 to 689
Object.defineProperty(Ember.String, 'htmlSafe', {
enumerable: true,
configurable: true,
get() {
deprecateImportFromString('htmlSafe');
return htmlSafe;
},
});
Object.defineProperty(Ember.String, 'isHTMLSafe', {
enumerable: true,
configurable: true,
get() {
deprecateImportFromString('isHTMLSafe');
return isHTMLSafe;
},
});
Copy link
Member

Choose a reason for hiding this comment

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

D'oh! Sorry about that, I totally missed it in the diff.

Since we're not calling the deprecation method, I thought they could all be assigned together.

I think that maybe it would be better to keep the infra around for the deprecation, and change the deprecateImportFromString to pass true to deprecate (with a nice comment explaining that it needs to be changed back to false when we have addressed that issue).

packages/ember/index.js Show resolved Hide resolved
@locks locks force-pushed the patch-htmlsafe-deprecation branch from b3d8861 to 4ee3d4c Compare February 11, 2021 02:20
@locks
Copy link
Contributor Author

locks commented Feb 11, 2021

I tried making the tests into todos but it didn't work 😭

@locks locks force-pushed the patch-htmlsafe-deprecation branch from 01c7dad to 07320fc Compare February 11, 2021 03:55
@locks locks force-pushed the patch-htmlsafe-deprecation branch from 07320fc to 7f70b81 Compare February 11, 2021 09:45
@rwjblue rwjblue changed the title Revert deprecation of htmlSafe and isHTMLSafe [BUGFIX release] Revert deprecation of htmlSafe and isHTMLSafe Feb 11, 2021
@rwjblue rwjblue added the Bug label Feb 11, 2021
@rwjblue rwjblue merged commit b005bda into master Feb 11, 2021
@rwjblue rwjblue deleted the patch-htmlsafe-deprecation branch February 11, 2021 15:00
@jordpo
Copy link

jordpo commented Feb 24, 2021

@rwjblue can we get a patch for 3.25 or should we just wait for 3.26?

@chancancode
Copy link
Member

Yes we plan on back porting this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants