Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Switch to purescript-react-basic? #47

Closed
kurtmilam opened this issue Jun 9, 2020 · 6 comments
Closed

Switch to purescript-react-basic? #47

kurtmilam opened this issue Jun 9, 2020 · 6 comments

Comments

@kurtmilam
Copy link

kurtmilam commented Jun 9, 2020

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
Copy link
Member

ajnsit commented Jun 10, 2020

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
Copy link
Author

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
Copy link
Member

ajnsit commented Jun 22, 2020

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

@kurtmilam
Copy link
Author

kurtmilam commented Jun 22, 2020

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
Copy link
Member

ajnsit commented Aug 2, 2020

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
Copy link
Member

ajnsit commented Aug 2, 2020

Closing this in favor of the umbrella issue.

@ajnsit ajnsit closed this as completed Aug 2, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

2 participants