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 beta] remove internal uses of Ember.String.fmt #10861

Closed

Conversation

justinwoo
Copy link
Contributor

In ES6, template strings are available so Ember.String.fmt can be
deprecated. This commit removes these usages and adds a JSDoc annotation
for deprecation.

fixes #10848

In ES6, template strings are available so Ember.String.fmt can be
deprecated. This commit warns against these usages so that they can be
safely migrated.

(I did try making this call Ember.deprecate, but too many tests fail when doing so. Would like help on this PR if that should be used instead.)

function fmt(str, formats) {
//Ember.deprecate('Use of Ember.String.fmt() is deprecated. Use ES6 template strings instead: https://babeljs.io/docs/learn-es6/#template-strings');
Ember.warn(strFmtDeprecationWarning);
Copy link
Member

Choose a reason for hiding this comment

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

Should be a deprecation not a warning, and all usage within Ember should be replaced with template strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I figured that should be the case. I changed it to Ember.deprecate and tried to chase it down (mostly just searching for fmt in packages) but I just don't know enough about Ember or the code base to track it down. Could you or someone walk me through some of the failing tests? (Currently sitting at 23 tests on my local browser QUnit)

@justinwoo justinwoo force-pushed the deprecate-ember-string-fmt branch 4 times, most recently from c058211 to 29e5e4e Compare April 10, 2015 03:45
@stefanpenner
Copy link
Member

:)

We should double check tomorrow at the team meeting, but I believe this will be without controversy

@stefanpenner
Copy link
Member

  • 👍 from the team
  • needs to become an add-on: ember-deprecated-string-fmt
  • internal usages should be removed

@justinwoo
Copy link
Contributor Author

add-on? you can monkey patch Ember using ember-cli addons?

@stefanpenner
Copy link
Member

you can monkey patch Ember using ember-cli addons?

yes, it is likely 1 file. We can distribute as both an ember-cli addon and a global file.

@justinwoo
Copy link
Contributor Author

I've gotten rid of all the internal usages of String.fmt, I can rebase and get rid of the deprecation warning in here.

I've only made a simple addon using blueprints... do you have any resources I should look at to get started?

@stefanpenner
Copy link
Member

awesome!

@justinwoo lets start with just the single file that does the polyfil. Either myself or @rwjblue or something can jump in this weekend and handle the add-on details themselves if you have trouble.

In ES6, template strings are available so Ember.String.fmt can be
deprecated. This commit removes these usages and adds a JSDoc annotation
for deprecation.
@justinwoo justinwoo force-pushed the deprecate-ember-string-fmt branch from 29e5e4e to 4c6e55d Compare April 11, 2015 18:50
@justinwoo justinwoo changed the title [BUGFIX beta] adds warnings against Ember.String.fmt usage [BUGFIX beta] remove internal uses of Ember.String.fmt Apr 11, 2015
@justinwoo
Copy link
Contributor Author

Rebased this to remove the deprecation from Ember.String.fmt, left it in JSDoc though.

Did I do this correctly? https://github.com/justinwoo/ember-deprecated-string-fmt

@mixonic
Copy link
Member

mixonic commented May 22, 2015

@stefanpenner this seems like a quick win for us to land, I expect someone on the cli team could champion its completion on that end.

I'm a little confused by the idea that the deprecation is in an addon. I though the pattern for these was deprecate in Ember, create an addon that silences the deprecation. I'll leave it to your discretion though.

@stefanpenner
Copy link
Member

would be great to get this in this week, so it makes the 2.0 cut.

@cibernox i suspect you are busy, but are you able to provide guidance / help here?

@pixelhandler
Copy link
Contributor

@stefanpenner when you say "needs to become and addon" I would like to kindly suggest "needs to become an addon, that includes global dist"

@stefanpenner
Copy link
Member

I would like to kindly suggest "needs to become an addon, that includes global dist"

Nothing prevents an add-on from becoming globalized as such I do not believe the globalized dist is a blocker for extraction. The globalization can happen at a later date without issue. Maybe a globalization task force, such as yourself can champion that effort. As mentioned before, it someone creates an addon blueprint that does that today, we can use that.

Otherwise it will be applied retroactively, as time permits.

@rwjblue
Copy link
Member

rwjblue commented Aug 2, 2015

I do not think we have the time to remove for 2.0.0, but I'm happy to land this as is (internal changes removing usage of Ember.String.fmt and soft deprecating), and leave a future cleanup effort in charge of removal.

Also, Looks like this needs a rebase.

@cibernox
Copy link
Contributor

cibernox commented Aug 2, 2015

I can take care of this today, very quickly if you're short of time @justinwoo

@stefanpenner
Copy link
Member

I can take care of this today, very quickly if you're short of time @justinwoo
Show all checks

would love that :)

@rwjblue
Copy link
Member

rwjblue commented Aug 3, 2015

Replaced by #11959.

@rwjblue rwjblue closed this Aug 3, 2015
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.

deprecated Ember.String.fmt
6 participants