fable-compiler / fable-react

Fable bindings and helpers for React and React Native
MIT License
275 stars 67 forks source link

Fable helpers for react-transition-group #127

Closed toburger closed 5 years ago

toburger commented 5 years ago

Hi, I am using react-transition-group for a fable project to get some animation effects into the application.

I'm still experimenting on how to define a good and easy to use API so any help would be very welcome!

This is my first contribution to an open source Fable/React helper so please be indulgent if I do make mistakes... 😇

Is it okay to make a PR into this repo? I think it makes sense to add it here.

TODOs:

MangelMaxime commented 5 years ago

HI @toburger

yes it's perfectly fine to create a PR in this repo :)

Because you are wrapping a React components my way to go in general it to do something similar to Fable.ReactLeaflet.

The idea is to have a DU per Props type and have all the possible valid option in this DU.

Also in general all the DUs have:

I like this way to go because like that if people use the Marker components for example. They can simplify do MarkerProps.* and see all the option possible without the need to dig into the documentation or code.

If you have "Du" inheritance with interface then some times you needs to apply casting and also then people can't discover easily the options for a specific components.

And the code duplication isn't a problem is you are creating a pure binding only (delivering only a .dll) as Fable will not include the sources into the generated bundle.

toburger commented 5 years ago

Thank you @MangelMaxime!

Your approach is much less error prone (makes impossible states impossible) and has a more user friendly API. My initial API was inspired by React.Recharts, but as you said it is less obvious which props can be applied to a certain element (Cartesian? or Polar?).

Question:

In react-transition-group CSSTransitionProps inherits from TransitionProps:

interface CSSTransitionProps extends TransitionProps {
    classNames: string | CSSTransitionClassNames;
}

Is there a way to inherit the CSSTransitionProps DU from the TransitionProps DU? As far as I know this is not possible in F#, but maybe I'm missing a special Attribute in Fable tho make this possible? Otherwise I'm simply redefining the DU cases. Shouldn't be that big of a deal...

MangelMaxime commented 5 years ago

Hum, I think it's possible to do that by using interface and making one inherit from another but it hard to reason about. And then if the user use mix properties he will needs to cast some of them to have the same interface used in the list.

For example

If you go the verbose approach you will not have this problem.

MangelMaxime commented 5 years ago

Otherwise I'm simply redefining the DU cases

This is exactly the approach I took for Fable.ReactLeaflet.

toburger commented 5 years ago

Okay thanks, I've updated the code to use separate DUs. Next I will try to get a sample to work.

toburger commented 5 years ago

I have a problem referencing the library with my sample project.

When I reference a file with a relative compilation include:

<Compile Include="../../Fable.React.TransitionGroup/Fable.Helpers.React.TransitionGroup.fs" />

I get the following error message:

ERROR in ../Fable.React.TransitionGroup/Fable.Helpers.React.TransitionGroup.fs
Module not found: Error: Can't resolve 'react' in 'C:\SourceCode\fsharp\fable-react\src\Fable.React.TransitionGroup'
 @ ../Fable.React.TransitionGroup/Fable.Helpers.React.TransitionGroup.fs 4:0-38 44:9-22 48:9-22 52:9-22 56:9-22 59:9-22
 @ ./src/App.fs
 @ ./src/Fable.React.TransitionGroup.Sample.fsproj

ERROR in ../Fable.React.TransitionGroup/Fable.Helpers.React.TransitionGroup.fs
Module not found: Error: Can't resolve 'react-transition-group' in 'C:\SourceCode\fsharp\fable-react\src\Fable.React.TransitionGroup'
 @ ../Fable.React.TransitionGroup/Fable.Helpers.React.TransitionGroup.fs 2:0-84 44:23-33 48:23-33 52:23-36 56:23-36 59:23-38
 @ ./src/App.fs
 @ ./src/Fable.React.TransitionGroup.Sample.fsproj

Also a project reference doesn't work (same error message):

<ProjectReference Include="../../Fable.React.TransitionGroup/Fable.React.TransitionGroup.fsproj" />

The reference to Fable.React works though.


The only way I got it to work is by copying Fable.Helpers.React.TransitionGroup.fs to the src directory and referencing it with:

<Compile Include="Fable.Helpers.React.TransitionGroup.fs" />

Am I missing something?

MangelMaxime commented 5 years ago

The problem is that Fable seems to wants react and react-transition-group to be installed inside C:\SourceCode\fsharp\fable-react\ folder which is this repo on your machine.

I think you should copy Fable.Helpers.React.TransitionGroup.fs directly into your test repo.

In general, when working on a library/bindings I work on a local version of the lib inside the my test project and then copy that file in the real repo library.

toburger commented 5 years ago

Okay, I've moved the existing sample project to a separate repository under https://github.com/toburger/Fable.React.TransitionGroup.Samples

toburger commented 5 years ago

I've added documentation by stealing some text from the transition group website and definitely typed typescript definition files.


OT

One thing that bothered me while I was adding the documentation: In React speech you commonly refer to a component as the thing that produces JSX. It is usually something stateful, where internal state is encapsulated in a component. Pure components are a special form of components, but not the default.

I think in Fable.React the word component isn't the right name because in Fable.React you generally define a function that produces JSX from its input. Pure functions are the preferred way to write this functions. Internal state should be communicated as something that you can do if you absolutely need to (like mutable in F#).

So I came up with the name element for the "thing" that produces JSX. What do you think? Is this renaming something that uncouples the Fable.React ecosystem too much from the underlying React ecosystem?

MangelMaxime commented 5 years ago

I think it is fine.

alfonsogarciacaro commented 5 years ago

Sorry, I saw @MangelMaxime was on this and didn't follow the conversation very closely 😅 Is this ready to merge (still has WIP in the title)? If so, do you want me to publish a package and make you a collaborator @toburger (or the other way around)?

toburger commented 5 years ago

I think the API COULD be a bit nicer, but at least it should be possible to use it in most scenarios. Didn't test it in more complex scenarios though.

I've created a sample site with some stolen examples to test some use cases here and I'm using it in a project at my company in production.

If you could publish it as a (alpha/beta) package and make me a collaborator would be great! 😄

MangelMaxime commented 5 years ago

@toburger Don't worry too much :)

When working on a project we always think it could be nicer :) But from my experience, as long as the user isn't stuck its fine we can refine it over time.

MangelMaxime commented 5 years ago

@toburger I published the package.

Is your name toburger for Nuget too ?

toburger commented 5 years ago

@toburger I published the package.

Awesome!

Is your name toburger for Nuget too ?

Yes