Show both internal and shared state managment in example. #7312
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I pointed someone at this example and they were confused by the shared state example. To help with their confusion I updated the example to show both shared and internal component state. There is an issue with the `IncrementGlobal` action that I've so far been unable to sort out. Upon first load, if you click the `Add` button beneath the `Persistan Count` display then the `IncrementGlobal` action seems to be dispatched twice on a single click. Hopefully, someone will be able to point out my, likely, very dumb error.
Stats from current PRClick to expand stats
Click to expand serverless stats
|
Stats from current PRClick to expand stats
Click to expand serverless stats
|
|
It might be worth using the context api since the new reason-react supports it. |
|
I looked into the context api but couldn’t determine where the attachment
point would be. How would I attach an app level piece of context into the
app level in Next.js?
On Sun, May 12, 2019 at 13:05 Adam Recvlohe ***@***.***> wrote:
It might be worth using the context api since the new reason-react
supports it.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#7312 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABZG4D4OJIJ65QTIQXPQ63PVB2BVANCNFSM4HMJ46GA>
.
--
Ciao,
Nathan Sculli
(702) 508-8678
|
|
There are a few domain specific configurations for Next.js. Based on the |
|
Thank you 🙏 I will look into that when I get back to my computer
On Sun, May 12, 2019 at 14:20 Adam Recvlohe ***@***.***> wrote:
There are a few domain specific configurations for Next.js. Based on the
redux example it looks as though you can create a _app.js the acts as a
wrapper for your components:
https://github.com/zeit/next.js/blob/master/examples/with-redux/pages/_app.js.
In this case, you would create your context in reason and then import it
into your _app.js file and pass it down using react context API.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#7312 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABZG4CGPO7GX6HE3UKEBKTPVCC3NANCNFSM4HMJ46GA>
.
--
Ciao,
Nathan Sculli
(702) 508-8678
|
…fix-for-issue-5049
… into rework-fix-for-issue-5049
Stats from current PRClick to expand stats
Click to expand serverless stats
|
Adding a custom hook to track the global state eliminates the double increment on first click issue and leads to all of the reference tracking being in the GlobalCount module. Also, this does not require the use of any odd concept within the components themselves.
Stats from current PRClick to expand stats
Click to expand serverless stats
|
Stats from current PRClick to expand stats
Click to expand serverless stats
|
|
@arecvlohe I've updated the implementation to use a custom hook. I muddled around with using the Context API but found that it was very confusing to me and required me to mix in a lot of JavaScript. Benefits of the custom hook implementation
Let me know what you think. |
|
@scull7 Let me play around with this tomorrow and get back to you. I think there is a nice way to work with context that doesn't require interop with JS. I kinda want to exhaust this possibility first. There is a nice example of context that I found in the discord channel so I will use that for inspiration. |
|
Why the desire to use the context API? The idea of it seems to run counter to one of the benefits of the hooks API. Namely removing extraneous wrapping from the DOM structure. The more I consider the global state reducer implementation; is this not what redux (and the Elm architecture) are doing implicitly? |
|
Well, I would like to explore this option. I am not necessarily tied to it. Just want to see what's possible. At the end of the day this is your PR so you can push it forward without any of my input but it would be nice to see if the API can be improved in any way. |
|
I apologize if I came off harsh in any way. I was asking so that I could
understand the reasoning.
On Wed, May 15, 2019 at 11:54 Adam Recvlohe ***@***.***> wrote:
Well, I would like to explore this option. I am not necessarily tied to
it. Just want to see what's possible. At the end of the day this is your PR
so you can push it forward without any of my input but it would be nice to
see if the API can be improved in any way.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7312>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABZG4BPZQKSJG3AXJEKYW3PVRL6PANCNFSM4HMJ46GA>
.
--
Ciao,
Nathan Sculli
(702) 508-8678
|
|
It's all good! I understand why you have some reservation about using context API since it might add more complexity than is necessary. My reservation is using a mutable reference as a means to hold state. You can say I am a bit of a purist in those regards. I am working on using the context API now and I should have something ready pretty soon. We can mull it over and see what makes the most sense. But this is still your PR so you can decide whether it's valuable or not. |
|
I look forward to seeing what you come up with. I understand the purity argument. But I think of the reference in this case as similar to using something like local storage, which for an example might cloud the purpose. Although, someone might also think that this is a good way to store state and that would probably not be good also... so 🤷♂ Also, while I generated the PR, I think this should be something more than just me thinks is a good idea. |
|
@scull7 I have a commit I would like to push to your branch. It's just 1 commit so you could easily remove it by rebasing. I just need permission to push to your branch and I can push up my changes. |
Stats from current PRClick to expand stats
Click to expand serverless stats
|
scull7
commented
May 15, 2019
examples/with-reasonml/pages/_app.js
Outdated
| const { Component, pageProps, store } = this.props | ||
| return ( | ||
| <Container> | ||
| <CountProvider> |
There was a problem hiding this comment.
I feel like this wrapping is exactly what the Hooks API is trying to avoid. Is there another way we can inject the provider?
There was a problem hiding this comment.
Also, we're injecting context in a place where it takes quite a leap to find.
There was a problem hiding this comment.
I don't think there is another way to inject the provider if we want it to work on every page without having to prepend it to the top level of index.re and about.re. I see this as a constraint of next being a JS framework and so have to work with its nuance.
As far as wrapping and how it relates to hooks, you still need a Provider, so I don't think there is a way to get around wrapping in terms of the context API. Maybe in the new version of reductive this will be solved.
There was a problem hiding this comment.
It seems like a lot of ceremony to avoid the use of a ref. I think the ref can be avoided if your willing to simply replace the state on each update.
Also, now we are introducing many more concepts, steps and indirections.
I get that you may have to use this file eventually, but is it necessary in this case? Should it be something that should be avoided unless really necessary? To me, it’s akin to using bs.raw
There was a problem hiding this comment.
There is definitely more boilerplate here and I don't really like that at all. At the same time, you can make the same comments about redux. But underneath all the boilerplate are some good principles. Not that what we have here is great but just like with redux you learn the patterns and you move on. So I kinda see it from that perspective.
examples/with-reasonml/pages/_app.js
Outdated
| @@ -0,0 +1,28 @@ | |||
| import App, { Container } from 'next/app' | |||
There was a problem hiding this comment.
This was the file I was trying to avoid adding.
There was a problem hiding this comment.
If you play around with next for any considerable amount of time you will probably use this file at some point. I don't see a reason to avoid it when it's provided to allow you to do this very thing.
| @@ -0,0 +1,27 @@ | |||
| module type Config = { | |||
There was a problem hiding this comment.
What does this file mean? It was quite difficult to explain this to a non-react user.
There was a problem hiding this comment.
This was the example I found in the discord channel. As far as what it means, it would probably require some discussion on functors in Ocaml/ReasonML but I would argue this file is not as important as CountContext.re which provides the API for how to create a global value using the context API. I could always add comments to this file that explains things more.
There was a problem hiding this comment.
Yes I agree that it will take some discussion of Functors, and module composition (because of the include). Also, this requires a lot of knowledge of React internals.
I think this is rather scary for someone new. I like this example because it does showcase some more advanced functionality. But perhaps it should be it’s own example? Removing the ref from with-reasonml and making the context example being with-reasonml-context ?
There was a problem hiding this comment.
Yeah, this is fine having it in a separate example. I am not that attached like I said before. It's was definitely interesting exploring the context API and how to implement it here.
Stats from current PRClick to expand stats
Click to expand serverless stats
|
|
@scull7 If you think it's better served as a separate example then I could create a new one entitled |
|
@scull7 Do you want to undo my last commit or do you want me to do it. I'll have some time to do it in the morning so just let me know if you want me to. |
|
I probably won’t have time until tomorrow night, so if you get to it before
I do then by all means. Thank you for the discussion on this. I also
enjoyed reading through your implementation and I learned a lot.
On Wed, May 15, 2019 at 20:23 Adam Recvlohe ***@***.***> wrote:
@scull7 <https://github.com/scull7> Do you want to undo my last commit or
do you want me to do it. I'll have some time to do it in the morning so
just let me know if want me to.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7312>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABZG4BXURTMCCUW2XRP5CLPVTHSVANCNFSM4HMJ46GA>
.
--
Ciao,
Nathan Sculli
(702) 508-8678
|
This reverts commit 03e7282.
… into rework-fix-for-issue-5049
|
@scull7 Reverted my commit so the branch is clean now |
Stats from current PRClick to expand stats
Click to expand serverless stats
|
|
Cool, thank you.
On Thu, May 16, 2019 at 11:44 Adam Recvlohe ***@***.***> wrote:
@scull7 <https://github.com/scull7> Reverted my commit so the branch is
clean now
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7312>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABZG4EVF6Y4EOSREXZHGGDPVWTQ7ANCNFSM4HMJ46GA>
.
--
Ciao,
Nathan Sculli
(702) 508-8678
|
Stats from current PRClick to expand stats
Click to expand serverless stats
|
|
I think this example is ready to be merged. Are there any objections? |
Stats from current PRClick to expand stats
Click to expand serverless stats
|
|
@lfades I believe this example update is also ready to go. |
lfades
approved these changes
May 23, 2019
This was referenced Jul 28, 2021
This was referenced Aug 31, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I pointed someone at this example and they were confused by the
shared state example. To help with their confusion I updated
the example to show both shared and internal component state
management.
I've changed the GlobalCount implementation to internally use a reducer hook and expose it as a custom hook. This keeps all of the global state tracking locked into the GlobalCount module and the Counter component just uses the custom hook as it would any other reducer hook.
I think this presents a clean pattern that doesn't introduce very many new concepts. As a bonus this fixes the duplicate increment on first click after load issue.