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

Add lazy type for lazy importing of models #1722

Merged
merged 14 commits into from
Aug 5, 2023

Conversation

clgeoio
Copy link
Contributor

@clgeoio clgeoio commented May 29, 2021

This PR is very much based on: #247.
It adds the lazy type, which can be used to dynamically import a model, it relates to the issue #1607

This PR still has a couple of todos:

  • Write out the documentation (I just copied it from late to make the test happy)
  • Look into the as unknown as T casting that is happening in the lazy function. Could there be a better type to use?
  • Look into the any that is being used in the shouldLoadPredicate. It'd be awesome if this could somehow infer the parent model
  • Add some error handling and associated tests

@clgeoio clgeoio changed the title wip: adds lazy type Add lazy type for lazy importing of models Jun 3, 2021
@clgeoio clgeoio marked this pull request as ready for review June 3, 2021 02:32
@clgeoio clgeoio changed the title Add lazy type for lazy importing of models Add lazy type for lazy importing of models [WIP] Jun 3, 2021
@jamonholmgren jamonholmgren added enhancement Possible enhancement has PR A Pull Request to fix the issue is available never-stale Should never be marked as stale by the bot require('@mweststrate') @mweststrate input is required! labels Jun 16, 2021
@clgeoio
Copy link
Contributor Author

clgeoio commented Jun 17, 2021

I'm still thinking about how to handle errors (e.g. if the loadType() promise rejects).
My ideas so far are:

  • Have onLoadError: (parent) => option in the lazy constructor so that the presence of an error can be communicated to the place where loading occurred.
  • Make the U generic conform to an interface with hasError: boolean so this can be set in via patch.
  • Expose the state of the loadType promise (resolved, rejected)

I expect that a root store may have a number of lazy types and to differentiate which stores loaded successfully may be nice

@iMakedonsky
Copy link

Awesome work!
Can't wait for it to be included in main repo

@clgeoio clgeoio changed the title Add lazy type for lazy importing of models [WIP] Add lazy type for lazy importing of models Oct 15, 2021
@jamonholmgren
Copy link
Collaborator

@mweststrate Do you have thoughts on this one?

Copy link
Collaborator

@jamonholmgren jamonholmgren left a comment

Choose a reason for hiding this comment

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

I'm okay with merging this. It's self-contained and seems well-written.

@jamonholmgren jamonholmgren removed the require('@mweststrate') @mweststrate input is required! label Mar 10, 2023
@jamonholmgren jamonholmgren added next-release Ready to go out in next release and removed never-stale Should never be marked as stale by the bot labels Mar 10, 2023
@coolsoftwaretyler
Copy link
Collaborator

Hey @jamonholmgren - last comment you made on this PR was that it's ready to merge. Should we do that? I'm not familiar with our overall release strategy. Do we merge this kind of thing directly into the master branch, or does it get on some separate release branch?

I'm guessing this is the PR we had that Twitter comment about.

@coolsoftwaretyler
Copy link
Collaborator

We have a related MR adding more types in #1960. I'm guessing we may get a conflict or two in docs files between this one and that one, but I'm hoping we can merge these in together and ship three new types total in the next minor version.

@coolsoftwaretyler
Copy link
Collaborator

Spoke with Jamon yesterday and we plan to ship an alpha minor version sometime next week. I'm going to merge this in now and target that release.

Thanks, @clgeoio! Sorry it took us so long here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Possible enhancement has PR A Pull Request to fix the issue is available next-release Ready to go out in next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants