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

Generated code #1

Closed
cmeeren opened this issue Oct 16, 2019 · 15 comments
Closed

Generated code #1

cmeeren opened this issue Oct 16, 2019 · 15 comments

Comments

@cmeeren
Copy link

cmeeren commented Oct 16, 2019

Replying to Shmew/Feliz.MaterialUI#22 (comment)

  1. I'll make sure to publish the generator project soon. In the meantime, the intended usage is to use Feliz.Generator as-is (as you would through a NuGet pakage), and then create a console app (like Feliz.Generator.MaterialUI) that uses it (and optionally extends it if you need that).

  2. One thing I notice right off the bat is that you seem to have one interface type for each property. That shouldn't be there. You simply use IReactProperty everywhere, and all prop implementations use Interop.mkAttr (from Feliz). So Types.fs and Interop.fs can be thrown out. Check out the Feliz.MaterialUI files Props.fs and Mui.fs to see how the output should look.

  3. You don't need async in Program.fs because you don't have any async code.


For very simple usage of Feliz.Generator, check out Feliz.Generator.CoreDom. Program.fs shows some base DOM components constructed manually. The output files show what the corresponding output should look like. Note that the example is special in the sense that it shows (for Zaid-Ajaj/Feliz#56) how to use it for native DOM props; For 3rd party libraries, please disregard the weird prop component that doesn't have any component overloads.


Let me know once you have had a look at the above (will give substantial changes), and I'll have another look.

@Shmew
Copy link
Owner

Shmew commented Oct 16, 2019

The reason I wanted to generate their own interfaces is because specific traces can have the same prop name like font but allows different or unique methods for that trace. If I remove the interfaces it doesn't allow type checking, but at the same time the only way this would happen is if you did something like data.scatter.font [...] inside a different trace like data.bar. The other thing is that outside the very top level Plotly.plot everything inside of it isn't a react element but just constructing the objects to pass as props into the component. I wouldn't want someone to try to use normal props within those functions.

I'm also pretty sure I can't pass anything besides the defined props into the component either.

Is there an actual disadvantage to have this many interfaces? I figured that all gets cleaned up when transpiled to js.

Yeah I just put everything into my project since there wasn't a package and I wasn't sure if I could do what I was looking to do without significantly modifying it.

Also one thing I would love to get your advice on how to resolve is when I have a nested type it doesn't seem to want to resolve the method, giving a compiler error, it's the section I had to comment out in this example. It says the method doesn't exist even when intellisense clearly shows it.

Thank you for looking at this so quickly, I really appreciate the feedback!

@cmeeren
Copy link
Author

cmeeren commented Oct 16, 2019

specific traces can have the same prop name like font but allows different or unique methods for that trace.

I have no familiarity with Plotly and don't know what a trace is. Can you give me a simple usage example of how you imagine the Feliz bindings to be (and in particular how they're different from the usage of "normal" components like DOM and Material-UI), and use that example to explain what you lose by removing the interfaces? It would be particularly helpful since it sounds from your description like Plotly isn't used syntactically like "normal" components such as those in Feliz.MaterialUI.

I could investigate Plotly myself of course, but I have my hands full. Help me help you 😊

I wasn't sure if I could do what I was looking to do without significantly modifying it.

Another indication that Plotly is "special". If it turns out you can't, then maybe the Feliz.Generator isn't the right solution (though depending on how it's different, maybe the generator can be extended/generalized somehow).

@Shmew
Copy link
Owner

Shmew commented Oct 16, 2019

Sorry, a trace us the way the data is presented on the graph. So for Plotly traces are bar, scatter, geo, polar, etc. Essentially a way of defining what type of graph it is, but it's done this way because in Plotly you can have multiple graphs in one such as a line bar graph.

So here is an example:

let chart () =
    Plotly.plot [
        plot.data [
            data.scatter [
                data.scatter.x [ 1; 2; 3; 4; 5]
                data.scatter.y [ 1; 6; 3; 6; 1 ]
                data.scatter.mode.markers
                data.scatter.name "Team A"
                data.scatter.text [ "A-1"; "A-2"; "A-3"; "A-4"; "A-5"]
                //data.scatter.marker [
                //    data.scatter.marker.size 12
                //]
            ]
            data.scatter [
                data.scatter.x [ 1.5; 2.5; 3.5; 4.5; 5.5 ]
                data.scatter.y [ 4; 1; 7; 1; 4 ]
                data.scatter.mode.markers
                data.scatter.name "Team B"
                data.scatter.text ["B-a" ; "B-b" ; "B-c" ; "B-d" ; "B-e" ]
                //data.scatter.marker [
                //    data.scatter.marker.size 12 <- this is what I was talking about above where it won't resolve the method
                //]
            ]
        ]
        plot.layout [
            layout.xaxis [
                layout.xaxis.range [ 0.75; 5.25 ]
            ]
            layout.yaxis [
                layout.yaxis.range [ 0; 8 ]
            ]
            layout.title [
                layout.title.text "Data Labels Hover"
            ]
        ]
    ]

Nothing in the above code can take any normal react element, making everything a IReactProperty would allow users to incorrectly input props that are not valid. As well as within the application there are differences between things like data.scatter.mode and data.polar.mode.

@cmeeren
Copy link
Author

cmeeren commented Oct 17, 2019

data.scatter.marker.size 12

That's a lot of nesting. I'm not sure how well the Feliz-style implementation works here (not saying it won't, just that I don't know).

In any case, this seems more to be a question of API design, not how to use the generator. if that is the case, I think @Zaid-Ajaj is the right person to ask.

@Zaid-Ajaj, do you have any high-level input on how the Plotly API can be designed?

@Shmew
Copy link
Owner

Shmew commented Oct 17, 2019

I actually was able to get it to work, and it doesn't require that amount nesting anymore (that the user can tell anyways) I have the changes reflected on the docs. I added EditorBrowsableState.Never to my own interop module but the big list of interfaces still show up when I type in Plotly. Shouldn't that not happen with this attribute?

@cmeeren
Copy link
Author

cmeeren commented Oct 17, 2019

The interfaces are defined directly in the namespace in Types.fs.

You need to add the attribute to all the interface definitions. Alternatively, you need to place them in a module and hide that module.

By the way, you don't have to have the Interop module at all. You can just use unbox (key, value) directly wherever it's needed, and make sure to specify the interface type: unbox<InterfaceType>

@Shmew
Copy link
Owner

Shmew commented Oct 17, 2019

Oh whoops, yeah that would do it, thank you. If there isn't a performance difference I'll probably keep the interop module since it feels cleaner to me. I was able to reduce the amount of visible nesting by relaxing the amount of interfaces I used the deeper into each module I got.

@cmeeren
Copy link
Author

cmeeren commented Oct 18, 2019

If there isn't a performance difference I'll probably keep the interop module since it feels cleaner to me.

As long as everything is inline then AFAIK there's no difference in the compiled JS.

@Zaid-Ajaj
Copy link
Contributor

@Zaid-Ajaj, do you have any high-level input on how the Plotly API can be designed?

Looking at the live examples in the docs, I think the API is nice to work with. Given that the plotly library (like any charting library) has so many options, this could be as good as it gets with regard to the nesting. I agree that "That's a lot of nesting" but it is consistent and it is easy to follow where you want to get the properties from. However, maybe it is an option to use a modified version of your proposed builder syntax to avoid the deep nesting:

/// What you proposed
Html.div (fun d ->
  d.style (fun s -> s.margin 5)
  d.children [
    Html.button (fun b ->
      b.onClick (fun _ -> printfn "Hello")
      b.text "Click me"
    )
  ]
)

Slightly modify into the following syntax but instead for Plotly.

Html.div (fun div -> [
  div.style (fun style -> [
     style.margin 5
  ])
  
  div.children [
    Html.button (fun button -> [
      button.onClick (fun _ -> printfn "Hello")
      button.text "Click me"
    ])
  ]
])

The latter syntax is much easier to implement, just run the handler function to get the list of properties, then createObj from it. Also it looks much more declarative than the former one. These overloads can be added along-side exisiting "original" Feliz API (with the public modules/classes) for experimentation.

@Zaid-Ajaj
Copy link
Contributor

On a different side-note, I believe that many React component libraries are usually like their own framework with each one having different ways to consume the API (think Recharts vs. Plotly/ApexCharts) so I am very skeptic when it comes to using a generic Feliz library generator because there will always be those weird libraries with weird APIs that the generator cannot account for. One specific generator per library would be the best option I think (along-side handwriting the bindings of course

@Shmew
Copy link
Owner

Shmew commented Oct 18, 2019

Thanks for input @Zaid-Ajaj I'll play around with the builder syntax to see how it works.

@cmeeren
Copy link
Author

cmeeren commented Oct 18, 2019

A downside of the lambda syntax is that you need to use instance members, which makes it impossible to have "combined" enum/non-enum props. It requires public-facing workarounds as per Zaid-Ajaj/Feliz#53 (comment).

On a different side-note, I believe that many React component libraries are usually like their own framework with each one having different ways to consume the API (think Recharts vs. Plotly/ApexCharts) so I am very skeptic when it comes to using a generic Feliz library generator because there will always be those weird libraries with weird APIs that the generator cannot account for.

That's certainly possible. I decided to factor out what seemed like useful common generator logic when I wanted to make Feliz bindings for react-select (not yet done). If the generator turns out to be useful there too, and for at least another project (rule of three), then I guess it may be generally useful.

In any case, I plan on having a closer look at Feliz.Plotly (no promises when) to see if it can be modified to use the current Feliz.Generator code as-is. If it stands up to the challenge, I guess a generalized generator may have some merit.

(Note that the generator can be extended/wrapped with custom functionality; for example, Feliz.MaterialUI needs to generate additional stuff not present in the core generator library.)

@Shmew
Copy link
Owner

Shmew commented Oct 18, 2019

Even if it isn't able to just plug and play (not saying it isn't) it was very helpful simply having something to start from. I suspect that it would be quite serviceable for any "normal" React library.

@cmeeren
Copy link
Author

cmeeren commented Oct 18, 2019

I just had a brief look at the Feliz.Plotly code. It seems complex enough (and different enough from "normal" React libraries, as you say) that I don't think I will look more closely at it after all. I'm pressed for time and have to prioritize where to spend my resources.

If you have any specific questions though, feel free to ask.

@Shmew
Copy link
Owner

Shmew commented Oct 29, 2019

I'm going to go ahead and close this now that the generated code is significantly less verbose and the api is in my opinion nice to use.

@Shmew Shmew closed this as completed Oct 29, 2019
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

3 participants