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

Ember.String deprecation RFC #236

Merged
merged 20 commits into from
Sep 26, 2017
Merged

Ember.String deprecation RFC #236

Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
980b0e6
Ember.String deprecation RFC
Serabe Jul 19, 2017
77f7305
Update on RFC
Serabe Aug 6, 2017
dcadbc1
Fix addon name
Serabe Aug 10, 2017
7bd7e66
Merge pull request #1 from Serabe/update-on-rfc
Serabe Aug 10, 2017
0cf1e6d
Update 0000-deprecation-ember-string.md
locks Aug 22, 2017
2c11102
Update 0000-deprecation-ember-string.md
locks Aug 23, 2017
ff9b190
Update 0000-deprecation-ember-string.md
locks Aug 23, 2017
f467aa1
Update 0000-deprecation-ember-string.md
locks Aug 23, 2017
ecd06ab
Minor fixes
Serabe Aug 23, 2017
51dfe48
Update 0000-deprecation-ember-string.md
locks Aug 31, 2017
575aa50
Update 0000-deprecation-ember-string.md
locks Sep 2, 2017
4d2f23b
Update 0000-deprecation-ember-string.md
Sep 7, 2017
4391801
Merge pull request #3 from bartocc/patch-2
Serabe Sep 7, 2017
a09d5f0
Update 0000-deprecation-ember-string.md (#2)
locks Sep 15, 2017
8a1eab0
Update 0000-deprecation-ember-string.md
locks Sep 15, 2017
762c9f8
Update 0000-deprecation-ember-string.md
locks Sep 15, 2017
84c5131
Fix minor wording
Serabe Sep 15, 2017
138ae8c
Update 0000-deprecation-ember-string.md
locks Sep 15, 2017
06d4761
Remove last appearance of @ember/component
Serabe Sep 19, 2017
c1aea6b
rename RFC file
locks Sep 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions text/0000-deprecation-ember-string.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
- Start Date: 2017-07-14
- RFC PR: (leave this empty)
- Ember Issue: (leave this empty)

# Summary

This PR proposes the deprecation of `Ember.String`. The following methods will be moved to an addon:

- `camelize`
- `capitalize`
- `classify`
- `dasherize`
- `decamelize`
- `fmt`
- `loc`
- `underscore`
- `w`

The remaining methods (`htmlSafe` and `isHTMLSafe`) will be moved to `@ember/component`.

In both cases, the String prototype won't be extended anymore.
Copy link

Choose a reason for hiding this comment

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

That's a pretty big breaking change. I know it's not great to extend prototypes but I know that at least our app pretty heavily relies on this. It would take quite some effort to audit and fix all the uses.

Perhaps this could be changed to:

In both cases, the String prototype won't be extended anymore by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a pretty big breaking change.

Yes? It will require a major version bump.

It would take quite some effort to audit and fix all the uses.

You can almost do this with a regular expression, but a codemod can definitely be whipped up.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can almost do this with a regular expression, but a codemod can definitely be whipped up.

@locks not so sure about this one. You can detect easily in a codemod a pattern like 'foo bar'.dasherize(). But hard to reliable transform a variable where we don't know what type it is (string or not), like:

function do(something) {
  return something.dasherize();
}

something could be an instance of some class, having a dasherize method completely unrelated to Ember.String.

So I think transforming your codebase could be a mainly manual step. Not saying that we shouldn't do this anyways! ;)

Copy link

@bryanaka bryanaka Jul 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@simonihmig You bring up a good point, but just to bring a different perspective, I don't think the point of a codmod is really to eliminate all human review of code change, but rather to remove the tedious step of changing everything yourself.

Use it, commit where you are sure the mod is correct, and review places you are unsure about.

I'm with @joukevandermaas on this - I rather have this extend string by default, but throw deprecations (wrap methods in a deprecation when merging onto prototype). Perhaps have a feature flag to turn off the string prototype extension.

Right now there is a feature flag for extending prototypes in general, but maybe this RFC breaks string prototype into it's own feature flag. I think that makes sense, because in my mind, each group of prototype extensions exist for a very different reason. I see String extensions as utility, where as function extensions are syntactical sugar, and array functions were a mix of syntactical sugar and polyfilling new Array methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the point of a codmod is really to eliminate all human review of code

@bryanaka Fully agree! But I think it should be quite conservative, in the sense that it may not convert everything, but for sure it won't do any harm, like making plain wrong transformations. Imagine a not so great test coverage, and wrong transformations are not detected, leading to real bugs...

I do think a codemod could add some real benefits when converting imports (new Ember Module API), or destructurings (const { String: { dasherize } } = Ember;) because they are easy to analyze statically. (See https://github.com/cibernox/ember-k-codemod). But I would question that a codemod would work well detecting the String prototype extensions, possibly doing more harm than adding value, as probably nobody writes 'foo bar'.dasherize(), so all use cases are comparable to the example given above (dasherize or any other method called on a variable rather than on a string literal) that you cannot reliably convert.

So tl;dr yes, the prototype extensions can only be removed for Ember 3.0, and while we are on 2.x the deprecation notices should be used to refactor our code, where possible with a codemod, but I think also to a quite large extend manually.


# Motivation

`Ember.String` was introduced long ago, even before 1.0 was released. It was a time without the current ecosystem of addons. There were no ES6 modules, no Ember CLI and no Ember addons. Global mode was the way to go and prototypes of classes like `String`, `Array` and `Function` were being extended.

A lot of nice-to-have functionality was added at that time but now Ember is lifting weight. These functions belong in an addon, where it can be maintained and evolve without being tied to the core of the framework.

Also, this would let people swap the function for similar implementations but different behaviour in edge cases.

Once most of the methods are moved to an addon and `String` is no longer being extended, makes more sense to move `htmlSafe` and `isHTMLSafe` methods to the component layer.


# Transition Path

Most methods will be extracted to an addon and will show a deprecation message when used from `Ember.String`. This way, if someone wants to use them, they just need to install an addon.

For the last two, similar approach but moving them to the `@ember/component` module.

# How We Teach This

Ember guides' section on _disabling prototype extension_ would need to remove references to these methods. `camelize` is also used in the services tutorial.

Main usage of these functions are in blueprints or connecting to APIs where converting the type of case of keys was needed. It is also used in Ember Data.

Furthermore, when people have moved to the new shims, moving to an addon would be as easy as to change the import path.

For `htmlSafe` and `isHTMLSafe`, the move is easier since the methods are easier related to components than to strings.

In any case, a basic Ember Watson recipe would be easy to provide.

# Drawbacks

A lot of addons that deal with names depend on this behaviour, so they will need to install the addon. Also, Ember Data and some external serializers require these functions.

`htmlSafe` and `isHTMLSafe` would need to change packages as well, thus the reason to try and provide an Ember Watson recipe.

# Alternatives

Leave things as they are.

# Unresolved questions