Ember.String deprecation RFC

[Serabe]

rendered

[ming-codes]

Aren't these used by Ember internally to convert camel case to kabobs case when resolving modules?

[btecu]

Ember data relies on those as well.

[stefanpenner]

IMHo is more important to get them off prototypes then it is to totally remove.

[joukevandermaas]

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.

[locks]

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.

[simonihmig]

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! ;)

[bryanaka]

@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.

[simonihmig]

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.

[locks]

@ming-codes @btecu @joukevandermaas @simonihmig @bryanaka I have overhauled the RFC, can you review and let me know if it's clearer?

[locks]

We discussed this at the September 1 core team call. We are generally all in favor of this moving forward and decided to move this to final comment period.

Thank you everyone for the comments, please review to make sure your concerns are addressed.

[Turbo87]

@locks the rfc gives no reason why the htmlsafe functions should be moved. can you explain that part?

[martletandco]

How does this impact helpers? htmlSafe and friends fall under templates rather than just components. You can import @ember/component into a helper but that shows it's a little off

Repeating comment here as requested

[Turbo87]

I agree with @martletandco. Importing htmlSafe from @ember/component seems very unintuitive compared to @ember/string

[jamesarosen]

There is a pattern in Glimmer (and in many modern typesafe environments) to export public interfaces from the consumer of that interface. Applying that pattern here, it might make sense to move htmlSafe not to @ember/component but to the template layer. In the long run, @ember/component might be the primary way in which Ember applications interact with the template layer. Currently, however, those interactions are split between components and routed templates.

[Turbo87]

I would like to propose moving the htmlSafe functions into a @ember/htmlsafe module instead of putting them in @ember/component to make the import seem a little bit more intuitive.

@locks @Serabe I'm still curious what the reasons are for wanting to move the functions out of @ember/string

[Serabe]

I like both @jamesarosen's and @Turbo87's ideas. @Turbo87's has an advantage: when/if routed templates are deprecated, the module does not need to be modified.

[sandstrom]

My thoughts:

• I agree with what others have said about @ember/component, it feels like an unintuitive place to move the functions.
• Very happy to see loc deprecated! Its existence was confusing and it's insufficient for translations anyway.
• Good idea to drop the string prototype extensions.

[alvincrespo]

I actually landed here after checking out emberjs/ember.js#15626. My first thought was:

Why is this going to be imported from @ember/component if the method names are related to Strings?

I don't see an explanation for that here, but I think the suggestion @Turbo87 made for imports such as @ember/htmlsafe makes the most sense. I think tightly coupling these "helpers" from the modules they are implemented in wouldn't be the most intuitive way. Just my 2 cents.

[martletandco]

Is there a namespace for template or rendering items? @ember/htmlSafe sets a trend of many small pieces to import. There should be some useful high level grouping

[ro0gr]

Thehe seems like typo

[locks]

Hey everyone, we have updated the RFC to address the concerns raised, namely the package where the HTML safe utilities should go.
Please review the current proposal, we will have another round of FCP. If no further concern is raised, it should be merged next Friday.
Thank you for participating!

[locks]

This RFC was discussed at a previous core meeting, and it seems the community is in agreement with how the concerns raised were resolved. Thanks for helping shape this RFC, time to .