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

Deprecate getWithDefault #554

Merged
merged 12 commits into from
Jan 31, 2020
75 changes: 75 additions & 0 deletions text/0554-deprecate-getwithdefault.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
- Start Date: 2019-11-08
- Relevant Team(s): Ember.js
- RFC PR: https://github.com/emberjs/rfcs/pull/554
- Tracking: (leave this empty)

# Deprecate getWithDefault

## Summary

Deprecate support for `getWithDefault` in Ember's Object module (@ember/object) because its expected behaviour is confusing to Ember developers.

## Motivation

The problem with `getWithDefault` is that its behaviour is confusing to Ember developers. The API will only return the default value when the value of the property retrieved is `undefined`. This behaviour is often overlooked when using the function where a developer might expect that `null` or other _falsy_ values will also return the default value.

Given the JavaScript language will soon (currently in Stage 3) give us the appropriate tool for this use case using the [Nullish Coalescing Operator `??`](https://github.com/tc39/proposal-nullish-coalescing), we can deprecate usage of `getWithDefault` and use that instead.
Copy link
Member

Choose a reason for hiding this comment

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

Some text should be added here: Ember can only suggest nullish coalescing syntax once Ember CLI supports that syntax out of the box. It should be supported in a stable release of Babel, but additionally we need to be sure that ember new includes that version of Babel.

And the deprecation instructions should explain how to get the proper version installed.


## Transition Path

Ember will start logging deprecation messages for `getWithDefault` usage.

We can codemod our current usage of `getWithDefault` with the equivalent behaviour using plain JavaScript. The migration guide will cover this example:

Before:

```js
import { getWithDefault } from '@ember/object';

let result = getWithDefault(obj, ‘some.key', defaultValue);
chrisrng marked this conversation as resolved.
Show resolved Hide resolved
```

After:

```js
import { get } from '@ember/object';

let result = get(obj, 'some.key');
chrisrng marked this conversation as resolved.
Show resolved Hide resolved
if (result === undefined) {
result = defaultValue;
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

let result = get(obj, 'some.key') || defaultValue;

Is this an alternative option? Also, should the codemod require get or can we use plain property accessors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to use get to minimize behaviour differences

Copy link
Contributor

Choose a reason for hiding this comment

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

Codemodding paths and interpolated paths will be significantly harder as well:

getWithDefault(obj, 'foo.bar');
getWithDefault(obj, `foo.${key}`);
getWithDefault(obj, path);

I'd also suggest to codemod towards get and then potentially run another codemod from get towards native accessors in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, there is already a codemod for get() to native: https://github.com/ember-codemods/es5-getter-ember-codemod. (at least for simple properties, no paths yet)

And someone even suggested covering getWithDefault() already: ember-codemods/es5-getter-ember-codemod#1 😝


### Nullish Coalescing Operator

The expected behaviour of the `getWithDefault` function is to only return the default value if it is strictly `undefined` so we cannot codemod usage directly into the nullish coalescing operator.

However [Babel](https://babeljs.io/) already supports the [nullish coalescing operator](https://babeljs.io/docs/en/next/babel-plugin-proposal-nullish-coalescing-operator.html) so we can use that for future use cases where we need to check if a property is `null` or `undefined` before applying a default value.

Similarly [TypeScript](https://github.com/microsoft/TypeScript) as of [version 3.7](https://devblogs.microsoft.com/typescript/announcing-typescript-3-7/#nullish-coalescing) also supports the operator so we will not be breaking that flow either.

## How We Teach This

Add the transition path to the [Ember Deprecation Guide](https://deprecations.emberjs.com/).

The references to `getWithDefault` will need to be removed from the [API docs](https://api.emberjs.com/ember/release/functions/@ember%2Fobject/getWithDefault).

There are no changes needed for the [Ember Guides](https://guides.emberjs.com/release/) since we do not use it anywhere.

## Drawbacks

The downside to deprecating `getWithDefault` would be an increase to the line length of component files that use it. This change will also cause some deprecation noise but could be mitigated with a codemod.

## Alternatives

### Adding `null` as a condition

We could add `null` as a condition alongside `undefined` which would return the default value provided. This is similar to what is proposed in [Nullish Coalescing for JavaScript](https://github.com/tc39/proposal-nullish-coalescing). This would however still be a breaking change since people who are depending on `getWithDefault` to work the way it does for `null` today will be broken if we change it.

### Do nothing

We could keep support in place, and provide more guidance around using it. There are already [some](https://dockyard.com/blog/2016/03/18/get-with-default) articles cautioning usage of `getWithDefault` when dealing with `null` or _falsy_ values.

## Unresolved questions

None at the moment.