purescript-concur / purescript-concur-react

Concur UI Framework for Purescript
https://purescript-concur.github.io/purescript-concur-react
MIT License
271 stars 17 forks source link

Switch to purescript-react-basic? #47

Closed kurtmilam closed 4 years ago

kurtmilam commented 4 years ago

purescript-react-basic seems to get some things right that purescript-react doesn't.

For instance, some of the props defined in purescript-react have types that make them either useless or less useful than they would be with more suitable types.

Examples of poorly typed props in purescript-react:

  1. *opacity variants are Ints, but should be either Strings or Numbers. These properties are almost useless as Ints, since they should accept all values in the "range 0.0 (fully transparent) to 1.0 (fully opaque)".
  2. This is similar but not quite as bad for stroke-width, which should be a string, as it accepts percentages (e.g. "50%") and 'lengths' such as "0.5px".

I understand these warts are outside of concur-react. It would be nice to use a react library that's well thought out. purescript-react-basic uses Strings for the props I mentioned above, and it supports many more props than purescript-react (e.g. transform, which I need).

Also, purescript-react-basic seems to be much more actively developed, and is under the lumihq organization, as opposed to being maintained by a single dev.

I don't have a handle on how much work would be required to swap out the underlying react library, or whether it would necessitate breaking changes, but I would contribute to that effort if you're interested.

ajnsit commented 4 years ago

Yeah that makes perfect sense. I've been meaning to swap out the libraries for a while myself.

It shouldn't be a lot of work at all. If you'd like to get started on it, I can give you some pointers -

The piece that does most of the work is a 15 line function Concur.React.componentClassWithMount which converts a Widget to a React component class. It should remain mostly the same for react-basic.

The rest of the files are functions that help you write widgets that target the react backend.

  1. You would need to change the definition of HTML to match the type of react-basic's ReactElement. The el* functions should remain the same. Then you can convert the wrappers for all the dom element types supported by React-Basic. Using the el* functions make it trivial. Check Concur.React.Dom for how it's done.
  2. Similarly you would need to change the definition of Prop to match the react-basic props. And then convert the wrappers in Concur.React.Props.

That should be it! Let me know if you have any questions!

kurtmilam commented 4 years ago

Unfortunately, it seems I have bitten off more than I can chew. I have been poking at this rewrite off and on the past couple of weeks and don't know that I'm really any closer to getting it working than when I first started, although I do feel that I'm getting a better handle on the purescript-react and purescript-react-basic APIs.

I'm still stuck trying to get React.purs / componentClassWithMount converted over to purescript-react-basic. I'm checking in Slack to see if someone would be willing to pair with me to get me moving in the right direction.

Any tips you have would be appreciated.

ajnsit commented 4 years ago

Do you have a repository with existing code I can look at?

kurtmilam commented 4 years ago

I do. I'm embarrassed to share it. It's in some Frankenstein state of me just trying to get types to line up, but here is where I currently am: https://github.com/kurtmilam/purescript-concur-react/tree/use-purescript-react-basic

Edit: See ReactNew.purs for my 'work'.

I also just realized that my latest set of changes was completely wrong (trying to turn component into a function), since purescript-react expects a constructur function, whereas purescript-react-basic doesn't. That's just an example of me pushing things around, slightly aimlessly.

I just pushed a couple of commits to the branch that do away with the unnecessary(?) component constructor function and put me back where I was earlier this evening.

ajnsit commented 4 years ago

Hey, I've been a bit busy the past month. I'll get around to it soon. Meanwhile it's open if anyone else wants to take a look.

ajnsit commented 4 years ago

Closing this in favor of the umbrella issue.