platform icon indicating copy to clipboard operation
platform copied to clipboard

NGRX Data - code examples not up to date to latest code and refer to Heroes and Villains but the data-sample-app has no mention of them

Open robc123 opened this issue 4 years ago • 15 comments

Background:

I am brand new to Angular, NGRX, NRWL, Github and a host of new goodies I am learning to do a full web stack; this is my first Github issue ever, so apologies if I have missed something. Thanks for providing code and documentation to help new starters to web framework software - I really like open source for that reason :-)

The issue:

All documentation under https://ngrx.io/guide/data refers to Heroes and Villains for code snippets and often has "refer to the full example app" type comments in it.
I have found one example snippet that does not appear to work when copied out which then makes me suspicious of the alignment between snippets and actual framework API functions e.g. the following fails:

/**
 * Filter for entities whose name or saying
 * matches the case-insensitive pattern.
 */
export function nameAndSayingFilter(entities: Villain[], pattern: string) {
  return PropsFilterFnFactory<Villain> ['name', 'saying'](entities, pattern);
}

Reported TS Fault:

Operator '<' cannot be applied to types '<T = any>(props?: (keyof T)[]) => EntityFilterFn<T>' and 'typeof Villain'.

My main problem is lack of experience in TS but then I cannot find Heroes and Villains mentioned at all in the example app to cross check it at: https://github.com/ngrx/platform/tree/master/projects/data-example-app

Simple fix needed:

[1] Add some tutorial and example code repo links to ngrx data overview; making it clear on status of that code in the documentation i.e. version of NGRX Data concepts it relates to. Ensure that covers the API adaption options like current docs, but make it clear if that is "current" or "needing work". In fact this would be good on all docs - in the overview page link to the relevant tutorial / finished code. [2] verify that the code snippets match the current framework API or extend the API page help to show examples e.g. add a good example here instead: https://ngrx.io/api/data/PropsFilterFnFactory

My Research

In trying to solve this myself, I did find the following that are not in NGRX repo but not sure how they are related to the main docs site as there are no links to them there:

  • https://github.com/johnpapa/angular-ngrx-data - this looks like an external add on that has been adopted into NGRX - under docs folder the code snippets do match your docs site - but last commit was 3 years ago so API changes not reflected? What is interesting is that here https://github.com/johnpapa/angular-ngrx-data/blob/master/src/app/store/entity/entity-metadata.ts there is a copy of the function that does not match the docs of that era, with no errors when copied and pasted:
export function nameAndSayingFilter<T extends { name: string; saying: string }>(
  entities: T[],
  pattern: string
) {
  return PropsFilterFnFactory(['name', 'saying'])(entities, pattern);
}

Other "finds":

  • https://github.com/johnpapa/heroes-angular - but this not match latest docs either e.g. nameAndSayingFilter example missing.
  • https://github.com/johnpapa/ngrx-data-lab/tree/finish gets you to see the very basic concept but not the full docs example again.

Other information:

I would be willing to submit a PR for the docs :heart:

[ ] Yes (Assistance is provided if you need help submitting a pull request) [x] No - I am just not experienced enough yet to commit to such a well used repo - even for docs - my example code would probably be worse!!

Thanks again for the hard work :-)

robc123 avatar Apr 27 '21 07:04 robc123

You're right, the code and the docs were migrated from johnpapa/angular-ngrx-data to @ngrx/data. We didn't migrate the example app, but recently created one in this repo, data-example-app.

If you notice that the docs are not up to date, a contribution is more than welcome.

timdeschryver avatar Apr 30 '21 15:04 timdeschryver

@timdeschryver, Do you know why the new data-example-app has gone to the Board of Stories structure as opposed to the Heroes and Villains structure of John Papa's? I think part of what @robc123 is finding confusing is all the example code in the docs uses the Heroes and Villians structure and some spots even include direct references to the 'accompanying' demonstration app with context implying it would be of the Heroes and Villians structure.

Should PR's for documentation work on migrating from Hereos and Villians structure towards the structure of the data-example-app? I think it would be helpful to those new to Data to have the docs and the example app a little more tightly coupled.

donohoea avatar May 17 '21 19:05 donohoea

@donohoea the data example app was added by a contributor because we didn't have an example app to incorporate ngrx/data. It also serves as a sandbox.

To be honest, I don't know how coupled the docs has to be with the example app. For ngrx/store and ngrx/effects, we used some snippets of the example app, and others were created for the docs.

I also don't know how up-to-date the docs of ngrx/data are, nor the heroes examples app. But we're always accepting (and are thankful) improvements and enhancements to the docs (and the example app).

I don't want to decide on my own which direction we want to take here, but we could open a discussion if that would help.

timdeschryver avatar May 20 '21 06:05 timdeschryver

@timdeschryver it looks to me like the docs for ngrx/data were written with specific refences to the a Hereos and Villians example app of John Papas. I think the simplest solution would be to bring the original example app into ngrx/data in addition to the existing data-example-app. Then we wouldn't have to remove references to it scattered through the docs. I'm open to a wider discussion and could look at updating and integrating the heroes example app at some point.

donohoea avatar May 20 '21 18:05 donohoea

I can take a look at updating the example and the docs to match if that is something that would help the community. I wrote the example app so would be more than happy to try and update to docs to match it. (and maybe clean up the code as well)

yharaskrik avatar May 21 '21 23:05 yharaskrik

I think if someone can align the docs with the current ngrx/data version and the example app, that would be great. I don't have an opinion on which example app we should use, as long as it uses the most used/important parts of ngrx/data. That being said, I do think that importing the tour of heroes examples would be the easiest 😅

timdeschryver avatar May 22 '21 15:05 timdeschryver

@timdeschryver do you have a link to the original tour of heroes demo app? I can try and migrate it today, if we do that, should I remove the data-example-app that i originally made since they accomplish the same task?

yharaskrik avatar May 22 '21 15:05 yharaskrik

This looks to be it? https://github.com/johnpapa/ngrx-data-lab/

That is the one that John Papa refers to in the original data repo anyway.

yharaskrik avatar May 22 '21 15:05 yharaskrik

@yharaskrik I was referring to this one in the angular-ngrx-data rapo.

Before you do, can we wait on @brandonroberts's thoughts please? 😅

timdeschryver avatar May 22 '21 15:05 timdeschryver

Ya no worries! I'll be on standby. :)

yharaskrik avatar May 22 '21 15:05 yharaskrik

I'm on board with bringing over the tour of heroes app

brandonroberts avatar Jul 21 '21 02:07 brandonroberts

Sweet. I can try and do that this weekend. Do we want to replace the example app I had in there then with the tour of heroes one?

yharaskrik avatar Jul 21 '21 04:07 yharaskrik

I'm in favor of replacing the existing example app with the tour of heroes.

timdeschryver avatar Jul 21 '21 07:07 timdeschryver

Good idea, I think that's the best course of action. Will update this thread when I get it done.

yharaskrik avatar Jul 21 '21 14:07 yharaskrik

PR opened.

yharaskrik avatar Aug 30 '21 04:08 yharaskrik