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

Thoughts #1

Open
zouloux opened this issue Jul 29, 2022 · 2 comments
Open

Thoughts #1

zouloux opened this issue Jul 29, 2022 · 2 comments

Comments

@zouloux
Copy link
Owner

zouloux commented Jul 29, 2022

Continuing discussion from this long thread :
zouloux/prehook-proof-of-concept#1

Answering @xdev1 :

@xdev1 : It would be really helpful, if the demos in reflex's README.md were written in TypeScript or at least a bunch of non-trivial examples.

True ! In fact, if this lib is used someday, I'll create a cleaner doc with categories, Readme.md will become a classic lib homepage with basic facts and pointing to the doc. For now, I'll keep it this way because it's still in beta and only used by me 🥸 I'll just create some code sandboxes so new users could play with it in plain TSX without having to setup an env.

@xdev1 : Also, using prettier would be very nice. Every syntax that is used with the reflex patterns should work fine with prettier, as a majority of developers (including me 😉) use prettier wherever possible.

Using prettier in the reflex/src ? Why not ! Again this is beta but I may accept to use prettier if I got others maintainers helping me. I'm not a huge fan on prettier on those kind of algorithmes, I often need manual formatting for better understanding. Also very opinionated but clearly not closed to prettier !

@xdev1 : I would consider, not to use the name hooks anymore if it's not "react-like" hooks. Folks will get confused otherwise (especially those who do not like react-like hooks, I know this from own bad experience in other discussions 😄). Maybe extensions could be a good name instead, as those functions are conceptionally extension functions where the first argument (= object) will just be passed implicitly.

Yep, I totally agree ! I was roaming into your https://github.com/js-works/preactive codebase recently and found the naming very interesting, I'll re-use this idea if you are OK 🙌 You are right about how things are named, this is very important for global understanding.

@xdev1 : One major goal for that factory-pattern based components should be to increase conciseness wherever possible and reasonable. In the old days we used stuff like this.props.label which was extremely cumbersome, React now uses just label which is awesome, while reflex uses props.label, which may look okayish first, but if you love React as I do and then you use reflex (or something similar), then even props.label seems way to noisy. I personally really prefer using the variable name p instead of props which reduces noise at a good amount especially in larger components. I know, normally we try to prevent single-character variables (except for maybe i, j, x, y etc.) but here this shortcut makes perfectly sense, IMHO. By the way, I'd also use s for a state object (i.e. s.count instead of state.count) and ctx for the used context values (i.e. ctx.theme).

I agree about how concise is React but I do not find this too much of a drawback. Solid has the same way of dealing with reactive values and props (as in a, object, not destructurable), and it feel fine when you know it. Will keep plain props in source code and examples because not everybody knows that p means props, and by extensions componentProperties. Also always destructuring objects into vars can be sometime noisy, a lot of curly braces to understand, which is fine for a lot of people but can be messy if done wrong. I'll maybe find a way to make defaultProps work with destructuring somehow (in stateless), if it's possible it will be implemented but I feel that stateful and stateless components should not work too differently. If any stateless component you are writing is becoming stateful for any reason, you should not be forced to refacto too much the code, it can be source of errors and frustration.

@xdev1 : Regarding stateless components: I would not provide some special API for those stateless components as it is not really necessary.

Stateless components are / will be very much the same as React functional components, but with the defaultProps and shouldUpdate functions, working like factory components. Maybe it will be through the preset and shouldUpdate extensions, which will work both in factory and stateless components. This can be easily explained in the doc + by raising errors in dev mode if misused.

@xdev1 : Frankly, I do not know, why count.set(count.value + 1) shall be better than setCount(getCount() + 1) or setCount(count() + 1) or count(count() + 1)

Yep that's pretty much the same BUT : Now states are usable both ways :

const myState = state( 0 )
state.value ++ // will trigger component update
// dom is not updated yet
await state.set( newValue )
// or
await state.set( v => v + 1 )
// dom is now updated and ready because of the await

Also, having state.value as a getter, will allow us to implement atomic rendering (diff only nodes that use this getter). It will have better perfs than React in a lot of cases and useState cannot do it because of value as a variable (not possible to track usages)
See @developit twitter thread on this : https://twitter.com/_developit/status/1549001036802625536

@zouloux : I'll search for a solution but the preset hook is maybe the only way to have this working in strict mode.

@xdev1 : The only type-safe solutions that I know are the following ...

@zouloux : I'm starting to like the preset version ! The HOC (curried) version feels more wrong to me because there is a lot of curly / destructuring / generic noise which can be avoided. It is less chars but it feels more to me ^^

}>({
  initialCount: 0
})((p) => {

As always, thanks for your time and your feedbacks ! You rock ! I'll try to re-read everything later because I'll get more subtleties. Also, I'm starting to use the lib on a personal project so I'll surely have some changes to do, will do better docs after this "alpha" phase is finished.

@xdev1
Copy link

xdev1 commented Jul 31, 2022

My question was more about, why this

// 98 chars
const count = state(0);
count.set(newValue);
count.set(count.value + 1);
count.set(v => v + 1);

shall be better than that?

// 82 chars
// "atomic rendering" is equally possible
const count = state(0);
count(newValue);
count(count() + 1);
count(v => v + 1);

I personally would use neither of them, but anyway...

And why does count.set(...) return a promise (instead of void)?
Are there any use-cases where this is really necessary?


Stateless components are / will be very much the same as React functional components, but with the defaultProps and shouldUpdate functions, working like factory components. Maybe it will be through the preset and shouldUpdate extensions, which will work both in factory and stateless components.

What's wrong with just using a memo function like React.memo?
If the component functions for stateless components are not pure any longer, like in React, that would be extremely odd and IMHO a really big anti-pattern.
Is it because shouldComponentUpdate is more powerful than React.memo, as you have also access to the component's internal state, and you need that additional power for whatever reasons?

@xdev1
Copy link

xdev1 commented Aug 3, 2022

@zouloux Please find here a little demo that uses "default props" and "should component update": » Demo
Counter1 uses the factory pattern with extensions, while Counter2 uses react-like hooks (I have not implemented a hook for "should component update" yet, but the extension version is called optimizeUpdates).
I think it does not make sense that the extensions preset and optimizeUpdates would also be allowed in non-factory component functions. I personally would just never (!) use props deconstructuring for components in the reflex style (only for components in the react style). So if you need "default props" or "should component update" you can very easily add this for any component.

BTW: That optimizeUpdates extension is in an early stage and currently not very developer friendly. This will change in future. But I guess it's okay for a POC here.

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

No branches or pull requests

2 participants