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

Type create/validate to accept an instance type #2199

Merged

Conversation

thegedge
Copy link
Collaborator

What does this PR do and why?

We've had issues with typings when instantiating references

CleanShot 2024-07-20 at 11 42 42@2x

#1568 for example, and maybe even #1764.

This PR resolves that by typing create to accept an instance type. I've added a "kitchen sink" test for create to test both snapshots and instances.

Steps to validate locally

The test added in this PR will type issues (like in above the screenshot) if you revert 6176e94.

@thegedge
Copy link
Collaborator Author

thegedge commented Jul 20, 2024

Hold off on a final review. I realized our tsconfig isn't properly typechecking tests, and this introduces a typing bug: it allows passing in a value for a view in model types.

Technically this works (it just gets ignored) but I'm thinking ideally we make it fail typechecking so users avoid making that error. This is actually tough to do, but I think we can do this by marking the getters as readonly and excluding those from the instance type in create.

I'm going to play with that idea, and if it works out, great! If it doesn't, I think I may remove the one test that's failing:

CleanShot 2024-07-20 at 12 16 39@2x

Do let me know if you think the tests I've added are comprehensive enough though! Want to make sure we have confidence in this change.

@thegedge thegedge force-pushed the instance-acceptable-for-create branch from 6176e94 to e1b5068 Compare July 20, 2024 16:37
@thegedge
Copy link
Collaborator Author

Will rebase this after #2200 merged, which should fix the typechecking / build stuff

@coolsoftwaretyler
Copy link
Collaborator

Hey @thegedge - just merged #2200 so you should be good to rebase and then figure out the new typing issues. Let me know when this is ready for another look.

@thegedge thegedge force-pushed the instance-acceptable-for-create branch from e1b5068 to f893791 Compare July 31, 2024 15:35
@thegedge
Copy link
Collaborator Author

@coolsoftwaretyler this is good for review now! <3

I was thinking we could also get this one in for #2201

@coolsoftwaretyler
Copy link
Collaborator

Perfect! Great idea. Will take a look sometime this week and get this into the next preview release.

@coolsoftwaretyler
Copy link
Collaborator

Alright, looks excellent and we will get this into the v7 preview. I think we'll consider this a breaking change, even though it should mostly be backwards compatible. I think type changes are just best treated as breaks overall, in case people are relying on the old type behavior.

@coolsoftwaretyler coolsoftwaretyler merged commit 3a2945d into mobxjs:master Aug 1, 2024
1 check passed
@thegedge thegedge deleted the instance-acceptable-for-create branch November 9, 2024 16:08
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.

2 participants