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

[WIP] Mock data source refactor and example#71

Open
orioljp wants to merge 1 commit into
developfrom
feature/mock-data-source
Open

[WIP] Mock data source refactor and example#71
orioljp wants to merge 1 commit into
developfrom
feature/mock-data-source

Conversation

@orioljp

@orioljp orioljp commented Oct 11, 2021

Copy link
Copy Markdown
Contributor

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

@orioljp

orioljp commented Oct 11, 2021

Copy link
Copy Markdown
Contributor Author

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.

Comment thread packages/core/src/repository/data-source/mock.data-source.ts

@doup doup left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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