Skip to content

refactor: Move CheckpointStore to Service Container #9

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

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

Conversation

a-drew
Copy link
Member

@a-drew a-drew commented Nov 26, 2021

Summary

Use the container to register concrete CheckpointStores against an interface.

Reasoning

This achieves the same singleton idea as implemented with static::store inside of Checkpoint Model but gives us a more convenient place to put these classes. Binding to the container works really well here as we in fact have an interface and concrete classes that would implement it. The container will let us typehint the store in any needed location and with singleton we can maintain a unique instance for the whole request lifecycle.

Testing

As a refactor, this change should maintain compatibility and keep all existing tests green

@a-drew a-drew changed the title Refactor: Move CheckpointStore to Service Container refactor: Move CheckpointStore to Service Container Nov 26, 2021
@kfriars
Copy link
Contributor

kfriars commented Dec 15, 2021

Before even reviewing, I just want to say I ❤️ this idea...

Copy link
Contributor

@kfriars kfriars left a comment

Choose a reason for hiding this comment

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

The only other thing I would consider here, is going one level up by using a facade to resolve the store out of the service container. Saves needing to do the app resolution all the time and you can type hint the methods on the facade.

@a-drew a-drew force-pushed the checkpoint-store-as-singleton branch from 03a1811 to 593d781 Compare December 15, 2021 21:33
@a-drew
Copy link
Member Author

a-drew commented Dec 15, 2021

The only other thing I would consider here, is going one level up by using a facade to resolve the store out of the service container. Saves needing to do the app resolution all the time and you can type hint the methods on the facade.

I think I can clean our those app() call by adding constructors in Checkpoint and CheckpointObserver and type hinting CheckpointStore as a required parameter 🤔 thoughts?

@a-drew
Copy link
Member Author

a-drew commented Jan 4, 2022

turns out models don't offer the type hinting from container magic 😢

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