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

Update documentation for get helper #1750

Merged
merged 3 commits into from
Dec 10, 2021

Conversation

MahemaS27
Copy link
Contributor

@MahemaS27 MahemaS27 commented Nov 30, 2021

This PR is motivated by a question that was surfaced by this issue in the emberjs repo .

The get helper use can be extended to objects with string or numeric keys, so arrays can also make use of the helper. This PR updates the docs to have examples of all of these use cases and have a more generic definition of what the get helper can do.

Copy link
Member

@ijlee2 ijlee2 left a comment

Choose a reason for hiding this comment

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

Hi, @MahemaS27. Thanks for opening a pull request.

I understand that a lack of documentation for {{get this.someArray 0}} may cause a developer to be stumped (maybe when working on an existing codebase).

However, I don't think the examples to be added are good practices that we should advocate in the Ember Guides. The API Guides specify that the 2nd argument must be a string, so {{get this.someArray "0"}} may be the correct usage.

The {{get}} helper is most useful when the key is dynamic. When the key is static, there are better, more readable ways to access the value:

{{!-- get helper is unnecessary --}}
{{get this.someObject "someProperty"}}

{{!-- My recommendation --}}
{{this.someObject.someProperty}}
{{!-- Proposed --}}
{{get this.someArray "0"}}

{{!-- Recommendation (1): Create a getter in the backing class and provide a meaningful name --}}
{{this.firstArrayElement}}

{{!-- Recommendation (2): Use {{object-at}} helper from ember-composable-helpers, or create a custom helper --}}
{{object-at 0 this.someArray}}
{{first-element this.someArray}}

As a result, I'd like to propose that we close the pull request. What do you think of the approaches that I provided above?

@jamescdavis
Copy link
Contributor

jamescdavis commented Dec 6, 2021

IMHO the API docs should be updated to indicate that the 2nd argument can be a number. This has been supported since Ember 2.14 and was added in emberjs/ember.js#15366 (along with a test to make sure it continues to work). This test still exists in Ember 4. Furthermore emberjs/ember.js#15667 fixed and added a test specifically for array element access via numeric index, which is still present, and emberjs/ember.js#16218 fixed this for a const number second argument and added a test for this, which is also still present. It's clear that get is intended to work with a number for the second argument and for accessing array elements by index given a numerical value.

@jamescdavis
Copy link
Contributor

API Docs source PR documenting numeric second arg and array element access: emberjs/ember.js#19871

Copy link
Contributor

@jamescdavis jamescdavis left a comment

Choose a reason for hiding this comment

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

A couple of suggestions.

guides/release/components/helper-functions.md Outdated Show resolved Hide resolved
guides/release/components/helper-functions.md Outdated Show resolved Hide resolved
@MahemaS27
Copy link
Contributor Author

Hi @ijlee2 , thank you for taking a look at my PR!

As @jamescdavis mentioned, this seems to be a supported behavior for get and has tests as well. Personally, I found the use of "0" to be confusing, as the integer 0 is an index of an array rather than a traditional key.

In regards to the examples you gave for alternative use, I think those are helpful and provide different ways to use the get helper. I would say that in the case of a simple array, its nice to rely on template logic rather than have to add more logic to a js or ts file. Also, array access via numeric index is very recognizable to most developers (for that specific case). I am fairly new to ember, so I was wondering why this is not considered best template practice? Thank you!

@MahemaS27 MahemaS27 requested a review from ijlee2 December 7, 2021 16:24
@ijlee2
Copy link
Member

ijlee2 commented Dec 7, 2021

In regards to the examples you gave for alternative use, I think those are helpful and provide different ways to use the get helper. I would say that in the case of a simple array, its nice to rely on template logic rather than have to add more logic to a js or ts file. Also, array access via numeric index is very recognizable to most developers (for that specific case). I am fairly new to ember, so I was wondering why this is not considered best template practice? Thank you!

@MahemaS27 Yep, accessing an array element with an index is common. I guess I've associated the {{get}} helper with accessing a POJO. As a result, {{get this.someArray 0}} felt like writing in JavaScript,

const { 0: elementOfInterest } = this.someArray;

return elementOfInterest;

But, of course, I'd prefer that we write,

const elementOfInterest = this.someArray[0];

return elementOfInterest;

I think a reason for using a getter or a helper such as {{first-element}} is readability. Code such as {{get this.someArray 0}} may not immediately make sense to everyone (i.e. what does 0 mean, in terms of syntax and maybe business logic? is an array zero- or one-indexed?). It is my perception that, with Octane, we prefer code that more people (designers, developers, and others) can understand.

@ijlee2
Copy link
Member

ijlee2 commented Dec 7, 2021

@MahemaS27 @jamescdavis Could one of you help with updating the API documentation for {{get}} (that a number is also supported) as a follow-up?

@jamescdavis
Copy link
Contributor

jamescdavis commented Dec 7, 2021

@ijlee2

Could one of you help with updating the API documentation for {{get}} (that a number is also supported) as a follow-up?

#1750 (comment) 🙂

Copy link
Member

@ijlee2 ijlee2 left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants