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

[JSX] Hack to allow plugins to specify a property is to be spread rather than constructed as a key,value pair #4038

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

shayanhabibi
Copy link
Contributor

This additional, non-obstructive branch statement in the JSX adds functionality that is particularly needed in my case to spread objects in JSX.

A plugin can provide an invalid attribute name "SPREAD_PROPERTY" in a property list to identify to the printer to skip the key and print the value enclosed in an object spread {...value}.

Just spit balling on how to implement it, but I would really like to push for something along these lines to work in JSX compilation.

@alfonsogarciacaro
Copy link
Member

This should be fine. Maybe using __SPREAD_PROPERTY__ instead? As we often surround magic words with two underscores. But it's not a big deal so if the plugin is expecting _SPREAD_PROPERTY_ it can go as is.

@shayanhabibi
Copy link
Contributor Author

shayanhabibi commented Feb 9, 2025

__SPREAD_PROPERTY__

Permits a value to be wrapped in obj spread syntax within JSX elements within the already defined property key,value schema.

ie, a property key,value pair of "__SPREAD_PROPERTY__", myProperties for some tag Tag will transpile to <Tag {...myProperties} />

This can cause undefined behaviour, exceptions, or errors if used on invalid objects within the JSX spec, so care must be taken by the user.

__BOOL_PROPERTY__

Based on mixed outcomes from different environments between some-attribute=true some-attribute="true" some-attribute={true} and some-attribute, there appear to be cases where one may cause issue over another.

Web components in particular, can use attributes as boolean switches, whereby the existence of the attribute key is truthy. This means some-attribute='', some-attribute="true", some-attribute="false" and some-attribute all result in a truthy outcome.

For completeness, I believe also adding a magic attribute key to output a property without any associated value is also worthy to consider in this PR.

ie, a property key,value pair of "__BOOL_PROPERTY__", "some-value" for some tag Tag will transpile to <Tag some-value />

MAGIC PROP KEYS

In general I imagine these will not be consumed directly by users of fable, but be made available as features through libraries and other DSLs.

Help wanted

Honestly, I'm not sure how to approach making tests in this environment for these kind of cases within the fable schema. I've scrolled through the React test framework, but I can't put together how the tests would function vis-a-vis compiling and comparing compiled output to provide flexibility in edge cases for plugins/users without polluting the test space with more libraries etc.

Would anyone be able to provide a sample that I can expand upon?

Documentation

I would like to provide documentation regarding these magics. Is there any central approach for documenting these for users and plugin writers?

@shayanhabibi
Copy link
Contributor Author

shayanhabibi commented Feb 9, 2025

@alfonsogarciacaro This PR is ready with tests; I will be able to produce tests for #4037 after this is merged as the tests will not work with #4037 (so I think) because there will not be an open/closed tag with #4037

@MangelMaxime
Copy link
Member

MangelMaxime commented Feb 9, 2025

@shayanhabibi Thank you for opening a PR.

One flow I see with the current proposition is that only 1 argument can use this system as at a time.

What I mean by that is what if a JSX.Component wants to use 2 __BOOL_PROPERTY__ ? With the current approach I think this is not possible.

Using an attribute to decorate the argument would allow such scenario, I am just not sure if the information is available in the AST at this point.

[<JSX.Component>]
let CounterJSXMagic(
		[<JSX.SpreadProperty>] foo:  obj, 
		[<JSX.BooleanProperty>] required : bool, 
		[<JSX.BooleanProperty("my-property-name")>] bar : bool
	) =
    JSX.html $"<div></div>"

@shayanhabibi
Copy link
Contributor Author

shayanhabibi commented Feb 9, 2025

As it currently stands, this is to enable plugins (specifically Oxpecker.Solid) to provide this functionality via some other API.

There is no easier way to produce these type of attributes within the current JSX generation/render phases otherwise without some deeper overhaul afaict.

Usage

In the example of Oxpecker.Solid, specific method usage is transformed into property expressions with the property key set to __SPREAD_PROP__. There is no limit to how many times this key can be provided by a plugin in a property key,value pair afaik.

[<SolidComponent>]
let Button ( so : SomeObject ) ( so2 : SomeObject ) =
    button(class'="someclass").spread(so).spread(so2) { "I'm the button text" }
<button class="someclass" {...so} {...so2}>I'm the button text</button>

Test

I stropped the magic properties in the test just to check that providing the magic key produces the desired outcome rather than that being the desired API usage for the functionality :P

@MangelMaxime
Copy link
Member

As it currently stands, this is to enable plugins (specifically Oxpecker.Solid) to provide this functionality via some other API.

I stropped the magic properties in the test just to check that providing the magic key produces the desired outcome rather than that being the desired API usage for the functionality :P

Hum ok I see.

In theory, if we were to use an Attribute it could perhaps be possible to change the identExpr instead of the property name in

code from your branch on Oxpecker

| "spread", { Args = [ _; identExpr ] } -> ("__SPREAD_PROPERTY__", identExpr) :: restResults

My issue, is that in general when we start accepting a "hack" solution then often it become a stable supported in the future because people start to use it.

IHMO, if we can make an implementation which don't rely on a hack it is for the best. Because, here we kind of cheat the F# type system because we are after it and Fable is kind enough to not do any check on the properties uniqueness :)

I didn't load the projects in an IDE so I don't know if have the information required to use attributes and how much work it would be to do that instead.

@shayanhabibi
Copy link
Contributor Author

shayanhabibi commented Feb 9, 2025

I'm not at all against that ideology.

To allow this same functionality to be used in the std Fable JSX seems doable.

Spread Prop

[<JSX.Component>]
let Button (prop : obj) ([<ParamList>] someObj : obj) = // ....

I imagine the ParamListAttribute could be used to indicate to the printer to handle the property in some manner. I'm not sure how to implement this effectively, but I could do some fiddling.

Bool Prop

[<JSX.Component>]
let Button (prop: obj) (required: JSX.PropBool) (someOtherBoolProp: JSX.PropBool) = //...

I Imagine a type alias to a boolean provided within Fable could be used to trigger a branch in the printer/renderer specific for boolean properties. The name of the identifier can then be directly translated to the attribute key.

Edit:
Ah I see that outside a plugin environment, I can do attributes for the properties (as you suggested before)
I'll go about this tomorrow/day after. Thank you!

In terms of being able to use it with the Oxpecker.Solid DSL/Plugin, I'm sure I'll figure something out :P.

@shayanhabibi shayanhabibi marked this pull request as draft February 9, 2025 17:21
@alfonsogarciacaro
Copy link
Member

Bool prop

Type aliases disappear in Fable AST, but if this general JSX behavior, we can just skip printing the value if it's a true literal (and skip the prop entirely if it's false).

Spread Prop

For the call site, we can indeed access the argument attribute and use for example ParamObject to define spread properties (we still need to pass this information to the printer somehow but changing the Babel AST is not a big problem because it's not exposed to plugins). Here:

let transformJsxCall (com: IBabelCompiler) ctx callee (args: Fable.Expr list) (info: Fable.MemberFunctionOrValue) =
let names =
info.CurriedParameterGroups |> List.concat |> List.choose (fun p -> p.Name)

My main concern is that, unless this is a feature only for declaring bindings to native JS components, resolving this in the declaration site requires, unless I'm mistaken, to flatten the spread props with the other props. This is trivial for records but other types maybe trickier.

@shayanhabibi
Copy link
Contributor Author

shayanhabibi commented Feb 10, 2025

Bool prop

Type aliases disappear in Fable AST, but if this general JSX behavior, we can just skip printing the value if it's a true literal (and skip the prop entirely if it's false).

This would work for HTML5, where the absence of a attribute means its 'bool' value is false, while the presence of the attribute is true, but is actually different in JSX, see https://stackoverflow.com/questions/60816731/how-are-boolean-props-used-in-react

Essentially, the absence of an attribute in JSX does not equate to false, but actually equates to undefined, at which time a default value may be inserted.

What this means, is that the absence of the attribute does not necessarily mean the same thing as providing the attribute a value of false. However, the presence of the attribute will equate to the attribute having a value of true.

To match this behavior, a true literal would result in skipping the printing of the value, but a false literal should result in a value of false. undefined would result in the property being skipped entirely.

This might not be something actually worth addressing, or enabling.

Spread Prop

For the call site, we can indeed access the argument attribute and use for example ParamObject to define spread properties (we still need to pass this information to the printer somehow but changing the Babel AST is not a big problem because it's not exposed to plugins). Here:

let transformJsxCall (com: IBabelCompiler) ctx callee (args: Fable.Expr list) (info: Fable.MemberFunctionOrValue) =
let names =
info.CurriedParameterGroups |> List.concat |> List.choose (fun p -> p.Name)

My main concern is that, unless this is a feature only for declaring bindings to native JS components, resolving this in the declaration site requires, unless I'm mistaken, to flatten the spread props with the other props. This is trivial for records but other types maybe trickier.

This is a functionality which hasn't been needed before, and I don't think is something worth addressing within Fable either, besides providing authors the ability to enter unsafe territory on their own terms and stipulations. It is something which I want, to be able to directly translate and port JSX to F# DSLs.

It is for declaring bindings to native JS components, but also to manipulate properties that are passed down component hierarchies when utilising headless libraries.

Boolean attributes don't function differently if used with boolean values in JSX (unless the UI library authors are sadistic. Material UI uses true|false|undefined to define the three states of the checkbox, where undefined is the third state), so i've never been left unable to achieve the same result in F# that is achieved in JSX. This is not true for spreading objects.

@shayanhabibi
Copy link
Contributor Author

shayanhabibi commented Feb 10, 2025

I'm a bit stuck at this point, I'm passing the attributes down the transformation tree, but I'm not sure how to approach it from here. Is there some way to tag or pass an attribute into the Expression type?

Edit: I see this breaks things in another call tree so this isn't an approach that would work. I keep coming back to hijacking the Printer attribute name :')

@alfonsogarciacaro
Copy link
Member

Bool prop

Ah, ok! Actually I just realized we are already skipping the prop if the value is null or undefined, so maybe we print the prop name only when the value is a true literal.

Spread prop

There's no standard way to pass info to the Babel printer, but we could add something to the JsxElement in the Babel AST. Another "hack" I was thinking of besides the magic words is to allow Emit for the JsxProps. For example, when Fable2Babel finds the ParamObject attribute, it transforms the prop name to something like emit:{...$0} and BabelPrinter resolves it like other emit templates.

@shayanhabibi
Copy link
Contributor Author

shayanhabibi commented Mar 5, 2025

I had a thought, but wouldn't this kind of work? It's more future proof. Passing an empty string "" would already compile and be invalid JSX. Might as well leverage that to provide flexibility.

Basically, an empty key "" in a key,value pair will result in the value being rendered directly. Either that or we should probably remove the capacity to use an empty string as a valid key,value pair altogether?

This allows myself, and any other plugin author to basically do whatever black magic they want. It also would have no impact on current behaviour of the printer, which should prevent any issues as a result of this implementation.

(Basically I got side-tracked with plugin development and don't have the time to implement this feature properly, but I really need it hahaha)

@shayanhabibi shayanhabibi marked this pull request as ready for review March 5, 2025 05:58
@MangelMaxime
Copy link
Member

Hello @shayanhabibi,

I am not a big fan on relying on an empty string to add behavior to codegen (if we were to do that we should add a comment in the code explaining what is happen).

Looking at the code JsxElement is defined as:

| JsxElement of componentOrTag: Expression * props: (string * Expression) list * children: Expression list

If we were to change props: (string * Expression) list to something like

type JsxProps =
	| Key of string
	| Bool of bool
	| Forward

props: (JsxProps * Expression) list

then we would be able to have a safe DSL at the printer level.

The issue/difficulty, seems to be that the plugin don't have access to the Babel Printer API, and this is instead transformed here:

let transformJsxProps (com: IBabelCompiler) props =
(Some([], []), props)
||> List.fold (fun propsAndChildren prop ->
match propsAndChildren, prop with
| None, _ -> None
| Some(props, children), Fable.Value(Fable.NewTuple([ StringConst key; value ], _), _) ->
if key = "children" then
match value with
| Replacements.Util.ArrayOrListLiteral(children, _) -> Some(props, children)
| value -> Some(props, [ value ])
else
Some((key, value) :: props, children)
| Some _, e ->
addError com [] e.Range "Cannot detect JSX prop key at compile time"
None
)

I suppose we don't want to add new cases to the type Expr from Fable.AST just for JSX. But what if like @alfonsogarciacaro mentioned we were to rely on Emit to pass information from the plugin to Fable.

Without modifying Fable.AST we could say if we detect this pattern then want to map to JsxProps.Forward

This would be done by adding helpers to Fable.AST package to make it easier to generate the expected "specialized" emit.

let jsxForwardEmit =
	Emit (
        {
            Macro = "jsx-forward-emit"
            IsStatement = false
            CallInfo = CallInfo.Create()
        },
        // We don't care about the typ
        Type.Any,
        None
    )

And then the plugin would using jsxForwardEmit as the key of prop when it wants it to be forward as is.

Doing so, also means we can have an equivalent for boolean props or any custom behavior we would want.

I don't know if this is better or not than the empty string to let the user pass anything he wants.

What are your thoughs @alfonsogarciacaro?

@MangelMaxime
Copy link
Member

I wanted to add that the empty string approach seems simpler and direct. In theory, like @shayanhabibi mentioned an empty string cannot be use as a key for a props.

And the approach I am proposing kind of feels more complex and I am not sure if this really have value, outside of making the DSL a bit more stronger.

@shayanhabibi
Copy link
Contributor Author

shayanhabibi commented Mar 5, 2025

I like the idea of upgrading the printer with a DU. The empty string thing is only because that undefined behaviour, as it were, was already present. But the printer with the DU is safer, and I can rule out empty strings being permissible. There's other things I would probably vote to rule out - prop keys shouldn't have spaces (if it wasn't visually disturbing, I was just going to use a prop key like "{...props} noop", Expr, which was how I was naively implementing the behaviour before).

Anything which doesn't impose this key=value absolute would be good.

I'm happy to either:

  • Implement the DU for babel printer, and the pattern matches for Fable2Babel
  • Stay with the empty string, and provide documentation
  • Disable an empty string being a valid prop key in Fable2Babel, and then pattern matching in Fable2Babel to emit an empty string key when Emit attribute is given, and forward the value expression on receiving an empty string on the BabelPrinter.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants