Deprecate Ember Utils

[snewcomer]

Plan to deprecate compare,isEmpty, isNone, isPresent, isBlank, tryInvoke, typeOf, notEmpty, none, empty. They will be extracted to an addon.

Rendered

Closes #333.

[sandstrom]

I think these methods are useful. They are quite a few Ember objects (DS.RecordArray, Ember.SafeString) that need custom logic to be checked properly for emptiness, length/blank, etc.

In a distant future we may be able to subclass native JS arrays and strings for all these cases, and at that point it may make sense to deprecate these. Until then we should keep them.

Also, I don't think the ecosystem churn caused by deprecating these is worth the gain in a slightly slimmer core.

[Turbo87]

Ember is moving towards a model where parts of the Ember codebase can be extracted to dedicated addons without any deprecations. Due to those plans I don't understand the motivation for deprecating functionality only to reintroduce it in a separate addon again.

[snewcomer]

So perhaps should this be reworded to state they may be deprecated in the future?

[bekzod]

this is also should mention corresponding computed properties empty, notEmpty, none because they rely on isEmpty,isNone,isPresent,isBlank

[balinterdi]

@bekzod I think the CPMs are still useful. If those were also removed, people would need to define CPs for them:

hasText: computed('text', function() {
  return !!this.txet;
}),

It's boilerplate code, but more error prone (see what I did there?) and less telling than hasText: isPresent('text').

Whether to have them in proper Ember or in an add-on is another question.

[bekzod]

@balinterdi was not suggesting to remove them, just suggesting RFC should mention what to do about them maybe

[robclancy]

These methods help annoying things in JS be less annoying. I would like them to stay.

[raido]

I don't think these utility methods are ready to go yet. I think these should be moved to separate package and nothing more at this point, opening up opt out possibility.

On the other hand I agree that code without these methods might be more readable as it is more explicit what type of values are checked. Currently engineers have to go digging in the API docs to understand how method X is supposed to work.

[robclancy]

On the other hand I agree that code without these methods might be more readable as it is more explicit what type of values are checked. Currently engineers have to go digging in the API docs to understand how method X is supposed to work.

I agree with this part but I think more because some of the utility methods seem poorly named. empty, blank and none could be interchanged with the functionality of each and still make sense. They don't explain what is going on. On top of that the docs do a poor job too.

isBlank is just isEmpty with a clear whitespace check.
isNone is just isNull in other languages.
isPresent is opposite to isBlank and something I don't think anyone should ever use. The docs for it also refer back to isBlank which has a doc of "A value is blank if it is empty or a whitespace string.". But what is empty? Now I need to go to isEmpty to find out. So checking docs for isPresent means you have to go to 2 other pages to find out what it is the opposite of.

I've now changed my mind. I think only isEmpty and isNone should stay since they are pretty standard things in other languages (but named different usually).

Personally I would rather isSet to do opposite of isNone and remove isNone, you would just use the inverse. Since None is a type in other languages and also doesn't really describe what it does. But something being set can be assumed to not be undefined or null.

And lastly I also support the rfc if the auto import library works well enough to not have any big downsides (I need to test build times) since I could just import these things from lodash.

[rwjblue]

Sorry for the delays here y'all, some replies below...

---

From @Turbo87:

Ember is moving towards a model where parts of the Ember codebase can be extracted to dedicated addons without any deprecations. Due to those plans I don't understand the motivation for deprecating functionality only to reintroduce it in a separate addon again.

There are two different things at play here. We are working towards enabling the codebase to be extracted, but we also want folks to directly add those addons as a dependency when it doesn't need to exist in "the core".

From @bekzod:

this is also should mention corresponding computed properties empty, notEmpty, none because they rely on isEmpty,isNone,isPresent,isBlank

Agreed.

From @robclancy:

These methods help annoying things in JS be less annoying. I would like them to stay.

I completely agree that these methods are convenient in some cases (though I think a number of other cases are better served by checking against your expected values). I don't think the goal is "remove them", but instead to move them into an addon that you would have as a dependency.

From @raido:

I don't think these utility methods are ready to go yet. I think these should be moved to separate package and nothing more at this point, opening up opt out possibility.

Agree, but the path to doing this is to deprecate using them from ember-source, and instead make you add the other package as a dependency. This is essentially the same path that we are working on for Ember.String...

[rwjblue]

FWIW, as far as I can tell, the RFC is already stating what I (and a number of others have said) in the comments.

The RFC mentions that these functions will be deprecated as part of ember-source, but extracted to another package for folks that want to continue using them (without deprecation)...

[NullVoxPopuli]

As long as I can still use these helpers with no changes other than to my package.json, I'm good with this

[elwayman02]

@snewcomer can we update this RFC to suggest codemods for adopting the extracted addon as another way to reduce friction as we transition? It may also be useful to do a code search to identify popular addons that rely on these utilities and encourage them to leverage vanilla JS alternatives to avoid forcing apps to opt-in to the extracted addon or lodash.

[snewcomer]

I've updated the RFC with everyone's comments! Based on what was said, it seems we are all in the same boat. In addition to helping implement this RFC, I'll work on a codemod right away (great idea @elwayman02) to make the transition as easy as possible.

[elwayman02]

Thanks @snewcomer! I think this RFC does a good job of covering the concerns brought up in my recent discussion; what's the next step to move this forward?

[elwayman02]

BTW, if you want to make the RFC title more succinct, you could name it something like "Deprecate Existence Utils & Computed Properties". That might make it easier for people to quickly understand what this is about rather than reading the names of all the methods.

[simonihmig]

@snewcomer Your Rendered link still points to an older commit, so it does not reflect your latest changes!

What about the import paths of the helper function from the new addon? As it seems to not cover all modules from @ember/utils, you will have to rewrite all those import paths, right? Or does it make sense to extract everything from (the pseudo package) @ember/utils into a real npm package, so the import paths can stay the same? 🤔

For the related CP macros, I also think they are quite useful and more readable than re-defining them all the time using a plain computed(). If they are extracted too, I am not sure if I would ever want to not add the addon - which kind of makes the extraction pointless. Also the choice of CP macros in core would be even more "unbalanced" IMO, as there are a few that I can't remember to have ever used (collect anyone?) which would remain in core (unless another RFC comes up), while a few of the more essential ones would be extracted here.

Re: the codemod, it could be clarified if it does one of the following transforms, or even both:

1. rewrite the import paths to the new addon
2. replace the helper function / CPs with an inline vanilla JS implementation

[elwayman02]

I disagree that the CP macros should stay in core. They are implementations using these utils, and as such as subject to the same problems in understanding the logic being executed. I don't use the CP macros any more than I use the utils themselves, and for the same reasons; they're confusing to developers who aren't intimately familiar with the internal implementation details, which does a lot to defeat their purpose. To be clear, removing these methods isn't just about making core slimmer; it's because they are actually problematic and hard to reason about.

[snewcomer]

@simonihmig Great point! 👍Codemod details need to be clarified. On point #2, my initial feeling is we would avoid rewriting to a vanilla JS implementation to avoid any surprises.

@locks @rwjblue

1. Do we want all utils moved out of core? I only listed 4/8 of them. I think the answer is to move all of them out.
2. Moreover, does it seem like pruning "all" of the computed macros might be a separate rfc? Jordan's point, while may have validity depending on your app, perhaps leads to too large of a changeset when it comes to implementation.

Overall I think all utils are extracted and computed macros are left so we can move forward with this RFC. Would love to hear everybody's opinions!

[mehulkar]

I personally love the CP macros, but after reading @elwayman02's point and also seeing our team use them inconsistently (usually just when it comes up in code review), I'm in favor of removing them as well. The two main features they offered me (over the better syntax) are:

1. automatically inferring dependent keys
2. safe navigation

Both seem like "nice to have" features, and can be seen as adding unnecessary surface area / learning curve.

👍 for merging this RFC.

[mehulkar]

I'm also in favor of answering @simonihmig's question here about what the codemod would do (replacing the code or updating the import). Either is acceptable, IMO.

[snewcomer]

👋 @pzuraq What are your feelings on this RFC? Willing to help move forward if the community thinks it is a good idea.

[pzuraq]

I don't think we should extract the computed property macros. These are tied very directly to computed properties as a whole, and it would make deprecating those harder. That will be a long deprecation path anyways, but I don't think it's worth causing extra churn in the meantime.

That would effectively block this deprecation until CPs are deprecated I believe, since the timeline would have to have the utils fully removed after CPs. I don't think this is necessarily a bad thing, since these utils are highly tree-shakeable on their own.

[jayzhou3]

I think these methods are useful. They are quite a few Ember objects (DS.RecordArray, Ember.SafeString) that need custom logic to be checked properly for emptiness, length/blank, etc.

In a distant future we may be able to subclass native JS arrays and strings for all these cases, and at that point it may make sense to deprecate these. Until then we should keep them.

Also, I don't think the ecosystem churn caused by deprecating these is worth the gain in a slightly slimmer core.

You should just use lodash because it provides more feature than what you think it is.

[MelSumner]

For this section, we are asking RFC authors to submit prose and code samples that can be used in the guides. Please update this section to reflect that. Thank you!

[pzuraq]

We discussed this in the core team meeting today, and after talking it over I think my original reservations were not actually meaningful. So I'm +1, and once Mel's comments are address and there is some prose for the deprecation guide added I think we can review it again!

Requested changes were addressed.

[snewcomer]

Happy to tackle this after 4.0 is released for 5.0 removal.

[chancancode]

We discussed this at today's framework and decided to put this into FCP!

[kategengler]

We discussed at the meeting today and since no new concerns have come up, we decided to proceed.