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

replace minimatch dependency with glob-to-regexp (#52) #55

Merged
merged 1 commit into from
Jun 17, 2018

Conversation

ioss
Copy link
Contributor

@ioss ioss commented Jun 16, 2018

This fixes (#52) the react-native problem by replacing minimatch with glob-to-regexp.

It should also shave of some KBs from the package.

I haven't really reviewed glob-to-regexp, but it seems fairly simple and all existing tests are still passing.

While this is a quick fix to be able to use react-automata in react-native without patching minimatch, I'd consider:

  • writing an even simpler matcher. Not sure, what kind of globs need to be supported.
  • while the values are only translated to a regexp once per call of the matches function, that should probably memoized?

@MicheleBertoli
Copy link
Owner

🙌 Thank you very much, @ioss.

I decided to use globs because I needed a way to describe values like:

  • all the children state of "a": a.*
  • any state that contains "error": *.error.*

In general, I don't recommend using the <State /> component as it couples the state machine with the UI - so we might even consider removing it and solve the issue once for all.
For now, I wouldn't create a custom DSL, but writing a simpler matcher is a good idea.

In the meanwhile, the package you found seems to work like a charm.
My only "concern" is about the performance and I strongly agree the RegExp should be memoized.
Would you be happy to submit a second PR using memoize-one?

@MicheleBertoli MicheleBertoli merged commit f53f876 into MicheleBertoli:master Jun 17, 2018
@ioss
Copy link
Contributor Author

ioss commented Jun 17, 2018

Well it should already be better as the version before as I pulled the pattern generation one level up.

Ok, nevertheless to memoizing:

Case 1: We have one value to match: memoize-one would be an option or a custom solution with "cache + key" on the instance too.
Case 2: We have a value array: memoize-one would invalidate it's cache constantly.

So the interesting case is case 2. I think, we should hook into the lifecycle of the component, as we only have to invalidate the cache, if the props of the components change. What do you think?

I noticed, that the Action component does not allow for glob-patterns. Is this intentional?
If we want to use the components lifecycle, there are multiple options:

  1. the cache is an instance of the component and Conditional is invalidating it. But I am not sure, what the future of Conditional holds (and it depends on the question concerning the Action component not accepting globs above)
  2. Conditional accepts a callback "valuePropChanged". Probably the way to go?
  3. some other idea from you?
  4. some others from me, that I do not like ;)

I'd be happy to create a second PR, after we have decided on the how.

@MicheleBertoli
Copy link
Owner

Well it should already be better as the version before as I pulled the pattern generation one level up.

+1, thanks!

I noticed, that the Action component does not allow for glob-patterns. Is this intentional?

Yes, because actions are labels/strings and they can't be nested.


I agree there are different places where we could apply the optimization, using memoization or a cache.
It would also be nice to restructure the Conditional component and move away from componentWillReceiveProps.
So, it might worth rethinking the approach first (v4), and then apply one of these solutions.

Thank you very much one more time for finding this glob library, @ioss.

@ioss
Copy link
Contributor Author

ioss commented Jun 18, 2018

Ok, let me know, if / when you need help.

@gutosanches
Copy link

Hey @MicheleBertoli any plans on making a release with this soon?

@MicheleBertoli
Copy link
Owner

Hey @gutosanches, I'm planning to make some other changes to the library (improvements, API breaking changes) and release a new major very soon.

@alexandrethsilva
Copy link

alexandrethsilva commented Oct 11, 2018

In general, I don't recommend using the <State /> component as it couples the state machine with the UI - so we might even consider removing it and solve the issue once for all.
For now, I wouldn't create a custom DSL, but writing a simpler matcher is a good idea.

@MicheleBertoli This actually brings up a question I have for a while now. It's true that the use of State couples the UI with the FSM, but for those new to FSMs and their use in UI, isn't it a fair compromise to have? It's still better than using nothing, right?

TBH, I have been using it myself for a while (all over, in fact) and given that I'm still getting started with FSMs, a completely decoupled FSM is still a bit far from what I can grasp right now.

I see the reasoning behind it, but I just can't visualise yet how it all would work and I'd even offer a PR to cover it in the README, but without knowing how though... I can't. 😄

Also, before reading this here and another comment on the xstate docs yesterday, I thought I was making a fairly correct use of things. 🤔😞

EDIT: For a bit of background, when I meant that not using State is still something I have a problem grasping, I was referring to something I raised here a while ago. Namely, sharing a single FSM across components. Despite your efforts to clarify it I unfortunately couldn't get my head completely around it. And without that, using component class hooks through the Action component is not possible.

Then again, may be just that I need to take some time and start from scratch in terms of my understanding of the library intents.

In any case, thanks for the help!

@MicheleBertoli
Copy link
Owner

Hey @alexandrethsilva,
using <State> is a fair compromise, it just makes things less flexible.

For example, you can't easily change the shape of the machine (e.g. adding a new state in between two existing state) without breaking the UI if you use <State>.
While actions tell your UI when to update, and they can be fired in any state (and can be moved from one state to another as well).

You mentioned redux in a comment, I recommend you give this a go - it might help understanding the decoupling concept.

Thank you very much!

@alexandrethsilva
Copy link

@MicheleBertoli I know I'm quite late here, but just wanted to thank you for the feedback. It didn't go unnoticed. 😄

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.

4 participants