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

Update antd #47

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

Update antd #47

wants to merge 11 commits into from

Conversation

steinybot
Copy link

@steinybot steinybot commented Aug 3, 2022

Issues:

@@ -103,14 +107,14 @@ object CSS extends js.Any
)
.columnsVarargs(
ColumnType[TableItem]()
.setTitle("Name")
.setTitle("Name": ReactElement)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunate side effect of the the union encoding in Scala.js. No double implicit conversions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it have a .setTitleReactElement or something like that?

@@ -174,11 +178,11 @@ object CSS extends js.Any
Select[String]
.defaultValue(selectValue)
.onChange((changedValue, _) => updateSelectValue(changedValue))(
Select.OptGroup.label("Manager")(
Select.OptGroup("Manager")(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess label changed from being optional to mandatory?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly

@@ -202,7 +206,8 @@ object CSS extends js.Any
Spin
.size(antdStrings.large)
.spinning(true)(
Alert(message = "Alert message title")
Alert
.message("Alert message title")
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vice versa here, message is now optional?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -410,7 +415,7 @@ object CSS extends js.Any

val renderTooltip = section(
h2("Tooltip"),
Tooltip.TooltipPropsWithTitleRefAttributes(title = ("Tooltip": ReactElement))(span("Hover me"))
Tooltip(TooltipPropsWithTitle("Tooltip": ReactElement).asInstanceOf[TooltipProps with RefAttributes[Any]])(span("Hover me"))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to do this? I forget.

Copy link
Author

@steinybot steinybot Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did TooltipPropsWithTitleRefAttributes used to come from? Is that a built in ST Slinky helper type?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit complicated. when the react components are generated sometimes we find components where the type of the props is a union type. two distinct set of valid props. the only way to encode that is to generate two components, so nesting is used for that. For instance ToolTip.PropsA .
Then there is the naming. typically these types are not explicitly named in typescript code, but we need them to be in this encoding. TooltipPropsWithTitleRefAttributes is likely derived from some combination of TooltipProps, WithTitle, RefAttributes. As you can imagine, when the converter comes up with these names it is consistent for the same code, but brittle across refactoring of the typescript code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: better way. I guess the underlying problem is that the generated component havent gotten correct props so you need to specify the whole props object, right?
You can explicitly create both and combine them to avoid the cast, TooltipProps(...).combineWith(RefAttributes[Any](...))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the long winded comment, it's more of a brain dump. I'm hoping this serves as some documentation for my future self when I inevitably forget how it works again. Don't feel obligated to reply to any of this. I'll convert these into separate issues at some point which will make it easier to comment on and either close or submit a PR to.


Ahh combineWith yes that is what I had a vague recollection of using.

It would be nice if there was a more convenient way to go from an A to an A with B if all the fields in B are UndefOr. I don't know if an implicit would work for this, at least not when B has type parameters.


What you said makes sense. I didn't click at first that Tooltip.TooltipPropsWithTitleRefAttributes was one of two components. It looked like it was the props. Naming is hard.

I found the reason for the change.

It was previously:

declare const Tooltip: React.ForwardRefExoticComponent<(TooltipPropsWithOverlay & React.RefAttributes<unknown>) | (TooltipPropsWithTitle & React.RefAttributes<unknown>)>;

Which generated:

typings.antd.components.Tooltip.TooltipPropsWithOverlayRefAttributes
typings.antd.components.Tooltip.TooltipPropsWithTitleRefAttributes

and there was no Tooltip.apply.

and now it is:

export declare type TooltipProps = TooltipPropsWithTitle | TooltipPropsWithOverlay;
declare const Tooltip: React.ForwardRefExoticComponent<TooltipProps & React.RefAttributes<unknown>>;

Which generates:

/* The props of this component has an unsupported shape. You can use `set` manually to use it, but with no compiler support :/ . Support for combinations of intersection and union types not implemented */
object Tooltip {
  
  def apply(p: TooltipProps with RefAttributes[Any]): Default[tag.type, js.Object] = new Default[tag.type, js.Object](js.Array(this.component, p.asInstanceOf[js.Any]))
  
  @JSImport("antd", "Tooltip")
  @js.native
  val component: js.Object = js.native
  
  implicit def make(companion: Tooltip.type): Default[tag.type, js.Object] = new Default[tag.type, js.Object](js.Array(this.component, js.Dictionary.empty))()
}

and

  /* Rewritten from type alias, can be one of: 
    - typings.antd.tooltipMod.TooltipPropsWithTitle
    - typings.antd.tooltipMod.TooltipPropsWithOverlay
  */
  trait TooltipProps extends StObject

The comment says "Support for combinations of intersection and union types not implemented" which is a little surprising since it was previously a union of two intersection types or does it mean something else?

I guess it would try and create TooltipPropsWithRefAttributes but that still wouldn't quite work since you couldn't pass a TooltipPropsWithTitle to that (without using combineWith). It would have to expand the TooltipProps type alias to get the same behaviour as before.

What is a bit confusing is how different it looks. In the former case if you want to create the component from the props you have to call either Tooltip.TooltipPropsWithOverlayRefAttributes.withProps or Tooltip.TooltipPropsWithTitleRefAttributes.withProps which means you have to know which one to use ahead of time rather than Tooltip.apply which you can provide either type of props (but you have to use combineWith to get it to the correct shape).


Could it be renamed to Tooltip.withProps rather than Tooltip.apply for consistency?


It might be nice if even when there are two components generated that it still has a Tooltip.withProps function like:

object Tooltip {
  def withProps(p: TooltipPropsWithOverlay with RefAttributes[Any] | TooltipPropsWithTitle with RefAttributes[Any]): Default[tag.type, js.Object] = new Default[tag.type, js.Object](js.Array(this.component, p.asInstanceOf[js.Any]))
}

That way you would always be able to create a component from the props which means you don't need to always remember how something gets encoded to use it (or have it break when someone replaces a union with a type alias like what happened here).


If the props are a supported shape then you get an additional builder which if I understand correctly does the same thing you can do with the builder you get for props. When it help the most is when the props is an intersection.

Ideally the API is as close to Slinky's API as possible. So instead of having a builder it would be an apply method taking the props as individual arguments. withProps is equivalent to the overloaded apply for class based components and component.apply for functional components.

There would still be the issue of encoding union props. For withProps this could be PropsA | PropsB as suggested above. For the apply method does overloading work? In the case of ambiguity, you could attempt to merge them or fallback to different names.


It also throws me off how the builder for the js.native traits use setX but for StBuildingComponent it is just x. I get why but it's something else to remember.

The other side to this is that having both setX and setXUndefined is really inconvenient if you already have an UndefOr[X].

We have found it much easier to use tap from scala.util.chaining such as:

props.tap(_.x = maybeX).tap(_.y = maybeY)

and still have the fluent builder style.


The part in the comment that says "You can use set manually to use it, but with no compiler support :/ " is also a little confusing because there is still a builder for both TooltipPropsWithTitle and TooltipPropsWithOverlay so you don't actually need to use set. It's the intersection that missing.


@steinybot steinybot marked this pull request as ready for review August 4, 2022 04:07
@oyvindberg
Copy link
Collaborator

Cool, thanks!

I'm short on time these days, but I'll try to schedule time to look at the issues you reported over the next days. then we'll see how many workarounds we need to keep here

@oyvindberg oyvindberg closed this Aug 4, 2022
@oyvindberg oyvindberg reopened this Aug 4, 2022
@oyvindberg
Copy link
Collaborator

doh, didnt mean to close

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

Successfully merging this pull request may close these issues.

2 participants