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

[WIP] Mock data source refactor and example #71

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

orioljp
Copy link
Contributor

@orioljp orioljp commented Oct 11, 2021

I updated the current MockDataSource with a version that I've been using in projects that I think it's much more versatile.

@orioljp
Copy link
Contributor Author

orioljp commented Oct 11, 2021

I marked this PR as WIP because It's still open to discussion. I added an example of implementation that is into the app scope, not harmony, just to show how it works and how clean is the DI.

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.

Thanks Uri!

I don't see any major issue. Although I've put few comments, please check them.

Anyway, this made me think that this is somewhat related to InMemoryDataSource; where the difference is that the MockDS would be pre-initialised by some default values (they could be randomly generated via your solution, or similar). Then, if we added pagination handling to the InMemoryDataSource it would be so much cooler. The wonderful world of "molaria". ^_^

}

protected get randUid() {
return Math.random().toString(36).substr(2, 9);
Copy link
Contributor

Choose a reason for hiding this comment

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

protected readonly delay = 0,
) {}

async get(query: Query): Promise<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, can you add explicit visibility to these methods?

Suggested change
async get(query: Query): Promise<T> {
public async get(query: Query): Promise<T> {

Comment on lines +76 to +78
value['id'] = this.randUid;
} else if (typeof value['id'] === 'number') {
value['id'] = this.randId;
Copy link
Contributor

Choose a reason for hiding this comment

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

At first this fooled me and I thought you had a bug & that you were setting the methods to value['id'], then I realised that both randId & randUid where using get keyword… maybe is clearer to just have the methods and to remove get?

Suggested change
value['id'] = this.randUid;
} else if (typeof value['id'] === 'number') {
value['id'] = this.randId;
value['id'] = this.randUid();
} else if (typeof value['id'] === 'number') {
value['id'] = this.randId();

Comment on lines +94 to +98
const randId = query instanceof IdQuery ? query.id : Math.floor(Math.random() * 1000);
return new UserModel (
randId,
`User_${randId}`,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes more sense to call this id:

Suggested change
const randId = query instanceof IdQuery ? query.id : Math.floor(Math.random() * 1000);
return new UserModel (
randId,
`User_${randId}`,
);
const id = (query instanceof IdQuery) ? query.id : Math.floor(Math.random() * 1000);
return new UserModel(id, `User_${id}`);

Comment on lines +89 to +90
// ---> App scope

Copy link
Contributor

Choose a reason for hiding this comment

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

This part could be extracted to the reference… not sure where though.

urijp pushed a commit that referenced this pull request Nov 10, 2022
Add `ngx-transalte` cache busting tip + other improvements
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.

4 participants