Skip to content

Conversation

@romamatusevich
Copy link
Contributor

getHeroes method is syncronous and returns this.heroes immediately. At the same time getAll method in BackendService returns resolved Promise that is why thenable chain should be returned and used.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

getHeroes returns this.heroes immediately.

Issue Number: N/A

What is the new behavior?

getHeroes returns thenable Promise chain.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Choose a reason for hiding this comment

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

This is good catch. I'm not sure if we need the explicit return type (PromiseLike<Hero[]>) as TypeScript will infer it for us. Personally I try to avoid adding info that TypeScript can figure out by itself, but I can see how it could be helpful for documentation / understanding.

WDYT?

Choose a reason for hiding this comment

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

Looking at the whole example again and suggested modification - the example is kind of broken now since it mentions cache but it doesn't use any caching in the way it is "written". So we would need to modify the code example to remove caching bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, maybe explicit type is not so useful here that is why I removed it. Also removed the comment regarding cache.

Choose a reason for hiding this comment

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

I'm assuming that we are talking about example on this page: https://angular.io/guide/architecture-components#introduction-to-components-and-templates , right?

If so I think that the example is fine as-is. It is just a simple code snippet that doesn't assume anything about the HeroService implementation, and specifically nothing about it being async.

Please revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

Thnx for this PR. I've left some comments I would like to see addressed.

`getHeroes` method is syncronous and returns `this.heroes` immediately. At the same time `getAll` method in `BackendService` returns resolved Promise that is why thenable chain should be returned and used.
@romamatusevich
Copy link
Contributor Author

Thnx for this PR. I've left some comments I would like to see addressed.

@pkozlowski-opensource, thank you for the comments. I've applied the changes. At the same time revert of changes in hero-list.component.ts leads to broken tests 'test_docs_examples' (hero-list.component.ts depends on getHeroes method in hero.service.ts). So, there are several options:

  • apply changes for hero-list.component.ts
  • leave everything as it was before and just close PR
  • make hero-list.component.ts and hero-list.component.ts independent from tests perspective aka remove tests (I personally against it)

@pkozlowski-opensource pkozlowski-opensource self-assigned this Aug 8, 2022
@ngbot ngbot bot added this to the Backlog milestone Aug 9, 2022
@jessicajaniuk jessicajaniuk added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Aug 9, 2022
@romamatusevich romamatusevich closed this by deleting the head repository Oct 19, 2023
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants