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
140 changes: 140 additions & 0 deletions text/0554-deprecate-getwithdefault.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
- 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) – both the [function](https://api.emberjs.com/ember/release/functions/@ember%2Fobject/getWithDefault) and the [class method](https://api.emberjs.com/ember/release/classes/EmberObject/methods/getWithDefault?anchor=getWithDefault) – 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
Sponsor 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);
```

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 😝


#### Using Nullish Coalescing Operator

We cannot codemod directly into the nullish coalescing operator since the expected behaviour of `getWithDefault` is to only return the default value if it is strictly `undefined`. The nullish coalescing operator accepts either `null` or `undefined` to show the default value.

The function `getWithDefault` **will not return** the default value if the provided value is `null`. The function will **only return** the default value for `undefined`:

```js
let defaultValue = 1;
let obj = {
nullValue: null,
falseValue: false,
};

// Returns defaultValue 1, undefinedKey = 1
let undefinedValue = getWithDefault(obj, 'undefinedKey', defaultValue);

// Returns null, nullValue = null
let nullValue = getWithDefault(obj, 'nullValue', defaultValue);

// Returns obj's falseValue, falseValue = false
let falseValue = getWithDefault(obj, 'falseValue', defaultValue);
```

The nullish coalescing operator (`??`) **will return** the default value when the provided value is `undefined` or `null`:

```js
let defaultValue = 1;
let obj = {
nullValue: null,
falseValue: false,
};

// Returns defaultValue 1, undefinedKey = 1
let undefinedValue = get(obj, 'undefinedKey') ?? defaultValue;

// Returns defaultValue 1, nullValue = 1
let nullValue = get(obj, 'nullValue') ?? defaultValue;

// Returns obj's falseValue, falseValue = false
let falseValue = get(obj, 'falseValue') ?? defaultValue;
```

This can be an option if we are aware that either `null` or `undefined` should return the default value.

Tooling Support:

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

- [TypeScript](https://github.com/microsoft/TypeScript), similarly, 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.

#### Using Object Destructuring With Defaults

If we would like to return the default value if the existing value is `undefined` we can also use [object destructuring](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment) with defaults.

Object destructuring with defaults **will return** the default value when the provided value is `undefined`:

```js
let defaultValue = 1;
let obj = {
nullValue: null,
falseValue: false,
};

// Returns defaultValue 1, undefinedKey = 1
let { undefinedKey = defaultValue } = obj;

// Returns defaultValue 1, nullValue = null
let { nullValue = defaultValue } = obj;

// Returns obj's falseValue, falseValue = false
let { falseValue = defaultValue } = obj;
```

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