Skip to content
This repository has been archived by the owner on Sep 5, 2023. It is now read-only.

FRONTEND-4243 :: Feature :: Harmony-typescript NetworkDataSource #131

Merged
merged 25 commits into from
Mar 6, 2023

Conversation

urijp
Copy link
Contributor

@urijp urijp commented Dec 31, 2022

Asana task link:
https://app.asana.com/0/1109863238977521/1202775968264243/f

Pull request information:
This PR contains de implementation of the generic network data source. The relevant changes here are:

  • NetworkDataSource implementation
  • NetworkQuery implementation
  • HttpRequestBuilder refactor, removing the mapping within this class to always use DataSourceMapper
  • Creation of two helpers to provide DefaultNetworkDataSourceMapper and DefaultNetworkDataSourceArrayMapper.

@urijp urijp requested review from doup and Alex-DA as code owners December 31, 2022 16:11
@urijp urijp requested a review from joselufo December 31, 2022 16:19
@lucianosantana
Copy link
Contributor

@orioljp why is it still flagged as WIP ? What is missing?

@urijp
Copy link
Contributor Author

urijp commented Jan 9, 2023

@lucianosantana yes, it's in progress because IMO we need an example or proper testing and this is not done mainly because I want to validate the approach with all of you before facing it. Could you check it @lucianosantana @doup @joselufo ?

@lucianosantana
Copy link
Contributor

@lucianosantana yes, it's in progress because IMO we need an example or proper testing and this is not done mainly because I want to validate the approach with all of you before facing it. Could you check it @lucianosantana @doup @joselufo ?

The approach seems correct to me.

@joselufo
Copy link
Contributor

I had a call with @orioljp, and I think the approach is correct. It has some differences with the Kotlin and Swift implementation, but its development makes sense.

Copy link
Contributor

@doup doup left a comment

Choose a reason for hiding this comment

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

Looks good in general, I've put "few" comments (🙏); and there are a couple of silly bugs. Maybe it would be nice to see some usage examples.

packages/angular/src/data/http-request.builder.ts Outdated Show resolved Hide resolved
packages/angular/src/data/http-request.builder.ts Outdated Show resolved Hide resolved
packages/angular/src/data/http-request.builder.ts Outdated Show resolved Hide resolved
packages/core/src/repository/query/network-query.ts Outdated Show resolved Hide resolved
packages/core/src/repository/query/network-query.ts Outdated Show resolved Hide resolved
packages/core/src/repository/query/network-query.ts Outdated Show resolved Hide resolved
packages/core/src/repository/query/network-query.ts Outdated Show resolved Hide resolved
packages/core/src/repository/query/network-query.ts Outdated Show resolved Hide resolved
urijp and others added 4 commits January 10, 2023 21:52
Co-authored-by: Asier Illarramendi <asier@mobilejazz.com>
Co-authored-by: Asier Illarramendi <asier@mobilejazz.com>
Co-authored-by: Asier Illarramendi <asier@mobilejazz.com>
Co-authored-by: Asier Illarramendi <asier@mobilejazz.com>
@urijp
Copy link
Contributor Author

urijp commented Jan 10, 2023

No problem, I appreciate it, as commented, it's a WIP and needs a deeper revision and testing, but your comments are useful, even apart from that, I need to add more things, so I'll ask for another check next week.

@orioljp
Copy link
Contributor

orioljp commented Feb 17, 2023

@lucianosantana @doup @joselufo @Alex-DA now you can check the whole PR, it's ready to be merged

Copy link
Contributor

@doup doup left a comment

Choose a reason for hiding this comment

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

Just two things (I've put them as comments also):

  • NetworkOneQuery should expose key (?)
  • I would like to see how query subclassing looks like as I've the impression that it can be improved, but I might be wrong.

Co-authored-by: Asier Illarramendi <asier@mobilejazz.com>
@doup
Copy link
Contributor

doup commented Feb 27, 2023

@urijp I'm approving it, if you do further changes and want another review tell me (or use the sidebar "Re-request review").

@orioljp orioljp changed the title [WIP] FRONTEND-4243 :: Feature :: Harmony-typescript DefaultNetworkDataSource FRONTEND-4243 :: Feature :: Harmony-typescript NetworkDataSource Mar 6, 2023
@urijp urijp merged commit 16aaf92 into master Mar 6, 2023
@urijp urijp deleted the feature/default-network-datasources branch March 6, 2023 16:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants