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

feat(data): add provideEntityData and provideEntityDataWithoutEffects #3647

Merged
merged 13 commits into from
Nov 7, 2022

Conversation

santoshyadavdev
Copy link
Contributor

@santoshyadavdev santoshyadavdev commented Nov 3, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

Closes #3529

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@netlify
Copy link

netlify bot commented Nov 3, 2022

Deploy Preview for ngrx-io canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 6076e41
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/636589c68ffb720008bd79b4

Copy link
Member

@markostanimirovic markostanimirovic left a comment

Choose a reason for hiding this comment

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

Thanks a lot Santosh for reopening this PR!

modules/data/src/entity-data.module.ts Outdated Show resolved Hide resolved
@markostanimirovic
Copy link
Member

I didn't add TSDoc and tests for the new data APIs yesterday, but it would be good to cover at least the main use cases. If you don't have time, we can add tests in a separate PR.

I'd also revert changes from standalone-app. We could improve the data-example-app project in the future and use standalone APIs there.

@santoshyadavdev
Copy link
Contributor Author

santoshyadavdev commented Nov 4, 2022

Sounds good I need to work on my slides. Should I disable these tests as of now and update once I am back?

Will update the standalone-app

@markostanimirovic
Copy link
Member

Hey @santoshyadavdev,

We don't need to exclude existing @ngrx/data tests. The suggestion above should fix existing tests (imports: [EntityDataModuleWithoutEffects] is currently missing).

@santoshyadavdev
Copy link
Contributor Author

Hey @santoshyadavdev,

We don't need to exclude existing @ngrx/data tests. The suggestion above should fix existing tests (imports: [EntityDataModuleWithoutEffects] is currently missing).

oops, my bad, really sorry, I am too tired 😂

Copy link
Member

@markostanimirovic markostanimirovic left a comment

Choose a reason for hiding this comment

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

oops, my bad, really sorry, I am too tired 😂

Oh 😅 No worries!

Copy link
Member

@markostanimirovic markostanimirovic left a comment

Choose a reason for hiding this comment

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

🚀

@brandonroberts
Copy link
Member

Changes look good overall. We have an opportunity to change the provideEntityDataWithoutEffects() to a better name.

I propose two options

  1. Rename to provideBaseEntityData() and keep provideEntityData()

  2. Use the pattern similar to the Angular Router

provideEntityData(config, [withEffects()])

Option 2 would be the default

@markostanimirovic
Copy link
Member

Changes look good overall. We have an opportunity to change the provideEntityDataWithoutEffects() to a better name.

I propose two options

  1. Rename to provideBaseEntityData() and keep provideEntityData()
  2. Use the pattern similar to the Angular Router
provideEntityData(config, [withEffects()])

Option 2 would be the default

Sounds good to me 👍 I'm also not a big fan of ...withoutEffects name 😅

Copy link
Member

@brandonroberts brandonroberts left a comment

Choose a reason for hiding this comment

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

LGTM.

We can change the signature in a follow-up PR

@santoshyadavdev
Copy link
Contributor Author

I can make changes tonight, if you want to wait 😃

@brandonroberts
Copy link
Member

Thanks Santosh! I think it's ok to go ahead and land this one. @markostanimirovic is going to look at the follow-up PR

@brandonroberts brandonroberts merged commit aa7ed66 into ngrx:master Nov 7, 2022
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.

Data: Introduce Standalone API for NgRx Data
3 participants