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.

Sign up for GitHub

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

Jump to bottom

Remove @ember/string methods from native prototype #19654

Merged
merged 2 commits into from
Aug 17, 2021
Merged

Remove @ember/string methods from native prototype #19654

merged 2 commits into from
Aug 17, 2021

Conversation

mixonic
Copy link
Member

@mixonic mixonic commented Jul 18, 2021 •
edited
Loading

Deprecated API removal per #19617

This patch does not removed the setting of String.prototype.htmlSafe, as installed at

ember.js/packages/ember/index.js

Lines 548 to 552 in a884ee5

if (ENV.EXTEND_PROTOTYPES.String) {
String.prototype.htmlSafe = function () {
return htmlSafe(this);
};
}
. I don't believe that was ever deprecated. Maybe we just missed it?

As such, this also does not completely remove ENV.EXTEND_PROTOTYPES.String

edit: The above caveat was discussed and you can read the comments here for the details.

@mixonic mixonic mentioned this pull request Jul 18, 2021
Closed
58 tasks
@mixonic mixonic requested review from pzuraq, locks and tomdale July 18, 2021 20:08
@mixonic mixonic marked this pull request as ready for review July 18, 2021 20:09
@mixonic mixonic mentioned this pull request Jul 19, 2021
Merged
rwjblue
rwjblue requested changes Jul 21, 2021
View reviewed changes
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Also needs to remove these things:

ember.js/packages/@ember/-internals/environment/lib/env.ts

Line 40 in ff16032

String: true,

ember.js/packages/@ember/-internals/environment/lib/env.ts

Line 221 in ff16032

ENV.EXTEND_PROTOTYPES.String = EXTEND_PROTOTYPES.String !== false;

ember.js/packages/@ember/-internals/environment/lib/env.ts

Line 225 in ff16032

ENV.EXTEND_PROTOTYPES.String = isEnabled;

@rwjblue
Copy link
Member

rwjblue commented Jul 21, 2021

As such, this also does not completely remove ENV.EXTEND_PROTOTYPES.String

Hmph. This is unfortunate. 🤔

@locks
Copy link
Contributor

locks commented Jul 21, 2021

Hmph. This is unfortunate. 🤔

I've added to this week's agenda so we have a think.

@mixonic
Copy link
Member Author

mixonic commented Jul 22, 2021

@rwjblue I can't do those removals until we are sure we can remove htmlSafe, right? This PR could land as-is, and we could do the htmlSafe removal and complete removal of EXTEND_PROTOTYPES.String in another PR.

Or we can block this until we figure out the path for htmlSafe. I'd rather decouple and just land this, but your review can make the call.

@mixonic
Copy link
Member Author

mixonic commented Jul 31, 2021 •
edited
Loading

At the July 23rd Ember Framework Core meeting we agreed that String.prototype.htmlSafe was intended to be deprecated. We don't expect it is in very high use across the Ember community, and we're comfortable expanding the string prototype deprecation to include it in a point release for 3.27 and in 3.28.0.

This PR should include String.prototype.htmlSafe in its removals, but before this is merged we should first write and land the PR to add it to the deprecations.

edit: See #19690

mixonic added a commit to mixonic/ember.js that referenced this pull request Aug 1, 2021
@mixonic
[BUGFIX release] Deprecate htmlSafe via prototype
4edd367 
In Ember 3.24 various string methods added to the `String.prototype` were
deprecated for removal in Ember 4.0. `htmlSafe` (the version available
via string prototype) was supposed to be included in those deprecations,
however dues to its implementation being different it was missed. This
omission can be understood as a bug.

This patch deprecates `String.prototype.htmlSafe` targeting Ember 4.0.
This will allow the removal of *all* string prototype extensions in 4.0
as intended by the original deprecation.

See also: emberjs#19654 (comment)
@mixonic mixonic mentioned this pull request Aug 1, 2021
Merged
mixonic added a commit to mixonic/ember.js that referenced this pull request Aug 1, 2021
@mixonic
[BUGFIX release] Deprecate htmlSafe via prototype
924405f 
In Ember 3.24 various string methods added to the `String.prototype` were
deprecated for removal in Ember 4.0. `htmlSafe` (the version available
via string prototype) was supposed to be included in those deprecations,
however dues to its implementation being different it was missed. This
omission can be understood as a bug.

This patch deprecates `String.prototype.htmlSafe` targeting Ember 4.0.
This will allow the removal of *all* string prototype extensions in 4.0
as intended by the original deprecation.

See also: emberjs#19654 (comment)
mixonic added a commit to mixonic/ember.js that referenced this pull request Aug 1, 2021
@mixonic
[BUGFIX release] Deprecate htmlSafe via prototype
b25e24b 
In Ember 3.24 various string methods added to the `String.prototype` were
deprecated for removal in Ember 4.0. `htmlSafe` (the version available
via string prototype) was supposed to be included in those deprecations,
however dues to its implementation being different it was missed. This
omission can be understood as a bug.

This patch deprecates `String.prototype.htmlSafe` targeting Ember 4.0.
This will allow the removal of *all* string prototype extensions in 4.0
as intended by the original deprecation.

See also: emberjs#19654 (comment)
mixonic added a commit to mixonic/ember.js that referenced this pull request Aug 1, 2021
@mixonic
[BUGFIX release] Deprecate htmlSafe via prototype
de3066d 
In Ember 3.24 various string methods added to the `String.prototype` were
deprecated for removal in Ember 4.0. `htmlSafe` (the version available
via string prototype) was supposed to be included in those deprecations,
however dues to its implementation being different it was missed. This
omission can be understood as a bug.

This patch deprecates `String.prototype.htmlSafe` targeting Ember 4.0.
This will allow the removal of *all* string prototype extensions in 4.0
as intended by the original deprecation.

See also: emberjs#19654 (comment)
mixonic added a commit to mixonic/ember.js that referenced this pull request Aug 1, 2021
@mixonic
[BUGFIX release] Deprecate htmlSafe via prototype
ca8666f 
In Ember 3.24 various string methods added to the `String.prototype` were
deprecated for removal in Ember 4.0. `htmlSafe` (the version available
via string prototype) was supposed to be included in those deprecations,
however dues to its implementation being different it was missed. This
omission can be understood as a bug.

This patch deprecates `String.prototype.htmlSafe` targeting Ember 4.0.
This will allow the removal of *all* string prototype extensions in 4.0
as intended by the original deprecation.

See also: emberjs#19654 (comment)
mixonic added a commit to mixonic/ember.js that referenced this pull request Aug 1, 2021
@mixonic
[BUGFIX release] Deprecate htmlSafe via prototype
d219784 
In Ember 3.24 various string methods added to the `String.prototype` were
deprecated for removal in Ember 4.0. `htmlSafe` (the version available
via string prototype) was supposed to be included in those deprecations,
however dues to its implementation being different it was missed. This
omission can be understood as a bug.

This patch deprecates `String.prototype.htmlSafe` targeting Ember 4.0.
This will allow the removal of *all* string prototype extensions in 4.0
as intended by the original deprecation.

See also: emberjs#19654 (comment)
mixonic added a commit to mixonic/ember.js that referenced this pull request Aug 1, 2021
@mixonic
[BUGFIX release] Deprecate htmlSafe via prototype
56126df 
In Ember 3.24 various string methods added to the `String.prototype` were
deprecated for removal in Ember 4.0. `htmlSafe` (the version available
via string prototype) was supposed to be included in those deprecations,
however dues to its implementation being different it was missed. This
omission can be understood as a bug.

This patch deprecates `String.prototype.htmlSafe` targeting Ember 4.0.
This will allow the removal of *all* string prototype extensions in 4.0
as intended by the original deprecation.

See also: emberjs#19654 (comment)
mixonic added a commit to mixonic/ember.js that referenced this pull request Aug 1, 2021
@mixonic
[BUGFIX release] Deprecate htmlSafe via prototype
c6c6978 
In Ember 3.24 various string methods added to the `String.prototype` were
deprecated for removal in Ember 4.0. `htmlSafe` (the version available
via string prototype) was supposed to be included in those deprecations,
however dues to its implementation being different it was missed. This
omission can be understood as a bug.

This patch deprecates `String.prototype.htmlSafe` targeting Ember 4.0.
This will allow the removal of *all* string prototype extensions in 4.0
as intended by the original deprecation.

See also: emberjs#19654 (comment)
@mixonic mixonic force-pushed the mixonic/string-prototypes branch 4 times, most recently from e27b0bb to 1673d98 Compare August 1, 2021 18:05
@mixonic mixonic force-pushed the mixonic/string-prototypes branch from 1673d98 to 8ccac17 Compare August 1, 2021 18:12
mixonic
mixonic commented Aug 1, 2021
View reviewed changes
packages/@ember/-internals/environment/lib/env.ts
@@ -36,8 +36,6 @@ export const ENV = {
*/
EXTEND_PROTOTYPES: {
Array: true,
Function: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

There are two commits in this PR, and the first makes this unrelated change to remove Function from this list and docs. The code deletion for function methods on prototype was done at #19663 but this removal was missed.

@mixonic mixonic requested a review from rwjblue August 1, 2021 18:48
kategengler pushed a commit that referenced this pull request Aug 2, 2021
@mixonic @kategengler
[BUGFIX release] Deprecate htmlSafe via prototype
8a4e4af 
In Ember 3.24 various string methods added to the `String.prototype` were
deprecated for removal in Ember 4.0. `htmlSafe` (the version available
via string prototype) was supposed to be included in those deprecations,
however dues to its implementation being different it was missed. This
omission can be understood as a bug.

This patch deprecates `String.prototype.htmlSafe` targeting Ember 4.0.
This will allow the removal of *all* string prototype extensions in 4.0
as intended by the original deprecation.

See also: #19654 (comment)

(cherry picked from commit c6c6978)
@mixonic mixonic merged commit 1beac1e into emberjs:master Aug 17, 2021
@mixonic mixonic deleted the mixonic/string-prototypes branch August 17, 2021 00:58
@nlfurniss nlfurniss mentioned this pull request Aug 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kategengler kategengler kategengler approved these changes

@pzuraq pzuraq Awaiting requested review from pzuraq

@locks locks Awaiting requested review from locks

@tomdale tomdale Awaiting requested review from tomdale

@rwjblue rwjblue Awaiting requested review from rwjblue

@MelSumner MelSumner Awaiting requested review from MelSumner

Assignees
No one assigned
Labels
Ember 4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mixonic @rwjblue @locks @kategengler