Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Allow EntityManager.create to infer EntityInstance type #364

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.

Sign up for GitHub

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

Jump to bottom
Open
bengreenier wants to merge 2 commits into
base: develop
Choose a base branch
Loading
from
Open

Allow EntityManager.create to infer EntityInstance type #364

bengreenier wants to merge 2 commits into from

Conversation

@bengreenier
Copy link

@bengreenier bengreenier commented Sep 27, 2023

Hi folks - I wanted to be able to omit the Entity type parameter on EntityManager.create, since I believe it can be inferred without issue. This PR attempts to allow just that.

Previously:

const user = new User();
user.id = '1';
user.name = 'Test User';
user.status = 'active';

const ret = await manager.create(user);
// ^? const ret: unknown

Now:

const user = new User();
user.id = '1';
user.name = 'Test User';
user.status = 'active';

const ret = await manager.create(user);
// ^? const ret: User

Please let me know what you think, and thanks for your hard work!

bengreenier
bengreenier commented Sep 27, 2023
View reviewed changes
packages/core/src/classes/manager/__test__/entity-manager.spec.ts
await expect(
manager.create<User>(userProperties as EntityInstance)
).rejects.toBeInstanceOf(Error);
await expect(manager.create<User>(userProperties)).rejects.toBeInstanceOf(
Copy link
Author

@bengreenier bengreenier Sep 27, 2023

Choose a reason for hiding this comment

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

since there is now a relationship between the type parameter Entity and the first parameter, we actually can't keep this test exactly as is.

The options I see are:

  • drop the User type parameter altogether
  • drop the EntityInstance cast
  • use EntityInstance as the type parameter and keep the EntityInstance cast on the input
  • drop all the type parameters and rely on inference (which will be inferred as the POJO type)

I choose to drop the cast, as that felt the cleanest to me, in expressing the failure intent (you can't pass POJO objects in when an Entity instance is expected).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@bengreenier
Morty Proxy This is a proxified and sanitized view of the page, visit original site.