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

[DEPRECATION canary] Deprecate Ember.String and prototype extension #15739

Closed
wants to merge 1 commit into from
Closed

[DEPRECATION canary] Deprecate Ember.String and prototype extension #15739

wants to merge 1 commit into from

Conversation

Serabe
Copy link
Member

@Serabe Serabe commented Oct 13, 2017

This PR deprecates the use of String prototype extension and
Ember.String namespace.

Methods coming from ember-runtime

A new StringUtils object is added, to be used internally by Ember
instead of using the same object being exposed as Ember.String. This
simplifies testing and the changes required in the rest of the code.
Also, dropping the deprecated code will be much simpler.

Methods coming from ember-glimmer

Deprecated versions are being added and reexported as the same global
the non-deprecated versions were. Besides, the non-deprecated version is
being exported under the new global Ember.Template.

The Ember.Template is just a placeholder until the final global is chosen.

@Serabe Serabe changed the title [DEPRECATION alpha] Deprecate Ember.String an prototype extension [DEPRECATION canary] Deprecate Ember.String an prototype extension Oct 13, 2017
@Serabe Serabe force-pushed the feature/deprecate-ember-string branch from 04075a9 to 37a06d4 Compare October 13, 2017 15:50
@locks locks self-assigned this Oct 16, 2017
@Serabe
Copy link
Member Author

Serabe commented Oct 23, 2017

@locks any update on this?

@Serabe Serabe mentioned this pull request Oct 23, 2017
Closed
@Serabe Serabe force-pushed the feature/deprecate-ember-string branch from 4abca4f to e27061b Compare November 17, 2017 18:16
@Serabe
Copy link
Member Author

Serabe commented Nov 17, 2017

restarting job. Seemed a problem with yarn.

@Serabe
Copy link
Member Author

Serabe commented Nov 17, 2017

Passing.

rwjblue
rwjblue reviewed Nov 18, 2017
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.

Looking really good! Only a few small questions/adjustments that I commented on inline...

packages/ember-glimmer/lib/utils/string.ts Outdated
{
id: 'ember-glimmer.ember-string-html-safe',
until: '3.5.0',
url: ''
Copy link
Member

Choose a reason for hiding this comment

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

Do we know the URL's for these deprecations? I vaguely recall that there was already an open website PR for them...

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding the URL from https://github.com/emberjs/website/pull/2995/files

packages/ember-runtime/lib/ext/string.js Outdated

const StringPrototype = String.prototype;

function deprecateEmberStringExtension(fn, opts = {}) {
let { name } = fn;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can rely on Function.name (AFAIK it isn't supported on all platforms, which would make this fail in production).

Checkout https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/name for a decent compat table (it isn't supported on IE at all)...

packages/ember-runtime/lib/system/string.js Outdated
@@ -115,6 +115,22 @@ function capitalize(str) {
return CAPITALIZE_CACHE.get(str);
}

function deprecateEmberStringUtil(fn, opts = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems roughly like the same function in the other file, can we share between the two and only have one implementation?

packages/ember-runtime/lib/system/string.js Outdated
@@ -115,6 +115,22 @@ function capitalize(str) {
return CAPITALIZE_CACHE.get(str);
}

function deprecateEmberStringUtil(fn, opts = {}) {
let { name } = fn;
Copy link
Member

Choose a reason for hiding this comment

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

Same concern as above RE: Function.name

packages/ember/lib/index.js Outdated
@type Object
@private
*/
Ember.Template = {
Copy link
Member

Choose a reason for hiding this comment

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

Lets underscore this, I'd hate for folks to start using it ....

@Serabe
Copy link
Member Author

Serabe commented Nov 19, 2017

Addressed your suggestions. Thanks for the review!

@Serabe Serabe force-pushed the feature/deprecate-ember-string branch 2 times, most recently from e6c86fc to cf9e15b Compare November 29, 2017 23:59
@Serabe Serabe force-pushed the feature/deprecate-ember-string branch 3 times, most recently from c257d81 to e0c7a5f Compare December 11, 2017 23:36
@Serabe
Copy link
Member Author

Serabe commented Dec 11, 2017

Rebased and squashed.

@locks locks changed the title [DEPRECATION canary] Deprecate Ember.String an prototype extension [DEPRECATION canary] Deprecate Ember.String and prototype extension Dec 11, 2017
@locks locks mentioned this pull request Dec 12, 2017
Closed
8 tasks
@Serabe Serabe force-pushed the feature/deprecate-ember-string branch from e0c7a5f to f7e9fca Compare December 22, 2017 13:32
@Serabe Serabe force-pushed the feature/deprecate-ember-string branch 2 times, most recently from fe778f1 to a672acb Compare January 5, 2018 17:47
@Serabe Serabe force-pushed the feature/deprecate-ember-string branch from a672acb to e9a150c Compare February 5, 2018 00:15
@Serabe
Copy link
Member Author

Serabe commented Feb 5, 2018

Having lots of timeouts while doing yarn install in CI

@Serabe
Copy link
Member Author

Serabe commented Feb 5, 2018

Yay! I got tests passing after restarting some jobs several times.

@Serabe Serabe force-pushed the feature/deprecate-ember-string branch from e9a150c to 5b80853 Compare February 7, 2018 13:19
@mmun
Copy link
Member

mmun commented Feb 25, 2018

@rwjblue should we merge this?

@locks
Copy link
Contributor

locks commented Feb 25, 2018

@mmun negatory, sir. we need to publish the deprecation guides and release the addon before.
sorry for dwelling on this 😞

@Serabe Serabe force-pushed the feature/deprecate-ember-string branch from 5b80853 to c034404 Compare March 16, 2018 16:00
@Serabe
Copy link
Member Author

Serabe commented Mar 16, 2018

Rebased!

@Serabe
Copy link
Member Author

Serabe commented Mar 31, 2018

I will rebase this at some point, quite likely, next Friday. Would that be ok, @locks ?

@Serabe Serabe force-pushed the feature/deprecate-ember-string branch from c034404 to d874956 Compare April 6, 2018 12:35
@Serabe
Copy link
Member Author

Serabe commented Apr 6, 2018

I redid the whole PR because this rebase was going to be harder than redoing everything.

@Serabe
[DEPRECATION canary] Deprecate Ember.String and prototype extension
4fc3dd5 
This PR deprecates the use of String prototype extension and
Ember.String namespace.

Methods coming from ember-runtime
---

A new `StringUtils` object is added, to be used internally by Ember
instead of using the same object being exposed as `Ember.String`. This
simplifies testing and the changes required in the rest of the code.
Also, dropping the deprecated code will be much simpler.

Methods coming from ember-glimmer
---

Deprecated versions are being added and reexported as the same global
the non-deprecated versions were. Besides, the non-deprecated version is
being exported under the new global `Ember.Template`.

The `Ember.Template` is just a placeholder until the final global is chosen.
@Serabe Serabe force-pushed the feature/deprecate-ember-string branch from d874956 to 4fc3dd5 Compare April 6, 2018 13:40
@Serabe
Copy link
Member Author

Serabe commented Apr 6, 2018

Green again.

@Serabe
Copy link
Member Author

Serabe commented Apr 13, 2018

Closing this. Sorry, I cannot keep rebasing this.

@Serabe Serabe closed this Apr 13, 2018
@locks
Copy link
Contributor

locks commented Apr 17, 2018

Sorry 😢I'll wait for #16530.

@kategengler kategengler mentioned this pull request Apr 5, 2019
Closed
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue approved these changes

Assignees

@locks locks

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Serabe @mmun @locks @rwjblue