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

Open
wants to merge 12 commits into
base: master
from

Conversation

@chrisrng
Copy link

chrisrng commented Nov 8, 2019

Rendered

@chrisrng chrisrng changed the title RFC Deprecate getWithDefault Deprecate getWithDefault Nov 8, 2019
chrisrng added 2 commits Nov 8, 2019
@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor

NullVoxPopuli commented Nov 8, 2019

I'm a fan. This is codemodable. 👍

chrisrng added 2 commits Nov 8, 2019
…ult.md
Copy link
Contributor

xg-wang left a comment

Should EmberObject.getWithDefaultalso be deprecated?

text/0554-deprecate-getwithdefault.md Outdated Show resolved Hide resolved
text/0554-deprecate-getwithdefault.md Show resolved Hide resolved
if (result === undefined) {
result = defaultValue;
}
```

This comment has been minimized.

Copy link
@snewcomer

snewcomer Nov 9, 2019

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?

This comment has been minimized.

Copy link
@chrisrng

chrisrng Nov 9, 2019

Author

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

This comment has been minimized.

Copy link
@buschtoens

buschtoens Nov 11, 2019

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.

This comment has been minimized.

Copy link
@simonihmig

simonihmig Nov 11, 2019

Contributor

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 😝

chrisrng and others added 2 commits Nov 9, 2019
Co-Authored-By: Thomas Wang <wangxgwxg@gmail.com>
Co-Authored-By: Thomas Wang <wangxgwxg@gmail.com>
@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Nov 11, 2019

Thanks for working on this @chrisrng! Seems like a good idea to me...

@amyrlam amyrlam mentioned this pull request Nov 12, 2019
10 of 13 tasks complete
Explicitly say both function and class methods are being deprecated
@EWhite613

This comment has been minimized.

Copy link

EWhite613 commented Nov 18, 2019

I don't think to have a code mod to do the following is really useful.

let result = get(obj, 'some.key');
if (result === undefined) {
  result = defaultValue;
}

I would suspect most people would modify the code mod to use their own util or use something like lodash (if they want that default behavior). Cause having one line turn into 4 while also increasing the complexity is not something I would think people would want in their code

@chrisrng

This comment has been minimized.

Copy link
Author

chrisrng commented Nov 19, 2019

FYI @steventsao added this to eslint-plugin-ember ember-cli/eslint-plugin-ember#594

@steventsao

This comment has been minimized.

Copy link

steventsao commented Nov 19, 2019

I don't think to have a code mod to do the following is really useful.

let result = get(obj, 'some.key');
if (result === undefined) {
  result = defaultValue;
}

I would suspect most people would modify the code mod to use their own util or use something like lodash (if they want that default behavior). Cause having one line turn into 4 while also increasing the complexity is not something I would think people would want in their code

Yeah turning one assignment into a block would not be ideal. But this can be a one-liner, right?

// I imagine the value of `get()` is cached 
const result = get(obj, 'some.key') === undefined ? get(obj, 'some.key') || defaultValue;

Optional chaining and nullish coalescing:

const result = obj.some?.key ?? defaultValue;
@rwjblue rwjblue self-assigned this Nov 19, 2019
@chriskrycho

This comment has been minimized.

Copy link
Contributor

chriskrycho commented Nov 19, 2019

@steventsao @EWhite613 Codemodding it in the safest way possible is the important bit here. You can definitely do it with a ternary, and that's fine with me as well. I suggested this particular output to @chrisrng because it seemed the clearest to me; a ternary would also be fine but does impose the slight extra cognitive overhead of having to think about the caching question when you're reading it. I'm not married to the let + if block for sure.

Copy link
Member

rwjblue left a comment

We discussed this in the Ember core team meeting today, and the team is broadly in favor of moving forward here. I think we need a few (hopefully minor) tweaks here.

We will likely need to at least mention nullish coalescing in the transition path, but also need to properly balance that against both the differences in getWithDefault's behavior (only uses undefined instead of null + undefined) and how destructuring with assignment would work (which uses "falsey" rules.

@chrisrng - Concretely, would you mind taking a crack at tweaking the transition path section to incorporate some messaging about destructuring, as well as some examples using nullish coalescing (basically explaining that there is a difference between it and getWithDefault, but if a particular case didn't care about the difference how it would look) and destructuring with defaults (same basic caveat, destructing with default uses "falsey" semantics).

text/0554-deprecate-getwithdefault.md Outdated Show resolved Hide resolved
text/0554-deprecate-getwithdefault.md Outdated Show resolved Hide resolved
text/0554-deprecate-getwithdefault.md Outdated Show resolved Hide resolved
text/0554-deprecate-getwithdefault.md Outdated Show resolved Hide resolved
chrisrng and others added 3 commits Dec 4, 2019
Co-Authored-By: Robert Jackson <me@rwjblue.com>
Co-Authored-By: Robert Jackson <me@rwjblue.com>
backspace added a commit to hashicorp/nomad that referenced this pull request Dec 5, 2019
Thanks to @johncowen for pointing out that this is on the
way to being deprecated: #5944 (comment)

emberjs/rfcs#554
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.