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

Refactored atoms to allow more funcionality #10

Merged
merged 16 commits into from
Nov 5, 2024

Conversation

brunocangs
Copy link
Collaborator

This PR breaks down the useMachineAtom function, separating it's internal functionalities in order to make them usable separately.

This PR closes #9 by introducing the atomWithActor function. It takes care of creating the Actor, based on the provided Logic and options. The type definitions correctly indicate when the Logic requires inputs, such as in the example added. It's setter function works exactly like the setter function for atomWithMachine

It also adds a atomWithActorSnapshot function, that handles the subscription logic to maintain the latest snapshot in state. Its setter function allows you to reset the subscription and clear its state whenever needed.

There is a type definition I had to bruteforce with a as any even though the input is correct and using the same type as their internal definitions it would always error for me

Copy link

codesandbox-ci bot commented Jul 12, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Thanks for maintaining the library!!

I hope someone can review the core part, but I'm afraid no one is available.

Here's some minor comments of mine.

examples/01_typescript/src/app.tsx Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@brunocangs
Copy link
Collaborator Author

@dai-shi I fixed the requested changes. Didn't realize there was a command to run the example with the vitest config, that's why it was failing for me 😄

@dai-shi
Copy link
Member

dai-shi commented Jul 15, 2024

a command to run the example with the vitest config

yeah, that was it.

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

This looks overall good to me.

But as I don't understand every details, it feels adding tests would be great and make maintainable.
Would you like to work on tests in this PR or in a separate PR?

@brunocangs
Copy link
Collaborator Author

This looks overall good to me.

But as I don't understand every details, it feels adding tests would be great and make maintainable. Would you like to work on tests in this PR or in a separate PR?

I feel like both tests and documentation are important for these changes. Either works for me, but I'm not sure how soon I'll have the bandwith to work on it. Possibly only in a few weeks

@dai-shi
Copy link
Member

dai-shi commented Oct 30, 2024

@brunocangs ?

@brunocangs
Copy link
Collaborator Author

@brunocangs ?

Hi,

I'm sorry for leaving this hanging for so long, I've added some basic tests to the new atoms, testing with a promise and testing listening to its events. Let me know if there are other tests you would like to see implemente

@dai-shi
Copy link
Member

dai-shi commented Oct 31, 2024

Hey, thanks for working on it.
I think we should ship it. Would you like to do it?

  • Merge this PR
  • Update CHANGELOG.md and update package version
  • Tag version and push it. (git tag v0.6.0)

@brunocangs
Copy link
Collaborator Author

@dai-shi also added some documentation and incremental examples for the new methods on the readme. Please let me know if you would like any changes.

If not I'll merge, tag and release it

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

LGTM (I really can't review from XState perspective though.)

Sure, go ahead.

@brunocangs brunocangs merged commit 582b941 into jotaijs:main Nov 5, 2024
2 checks passed
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.

req: Allow access to internal service
2 participants