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

Support displayName in Scala functional components #1092

Merged
merged 6 commits into from
Mar 6, 2024

Conversation

rpiaggio
Copy link
Collaborator

@rpiaggio rpiaggio commented Mar 3, 2024

  • ScalaFnComponent and ScalaForwardRef now support .withDisplayName(name). Must be called first (before hooks/render*/withChildren*).

Example:

  ScalaFnComponent.withDisplayName("MyComponent")
    .withHooks[Props]
    .useState(0)
    .render((_, s) => s.value.toString)

I disabled Mima checks temporarily since this is binary incompatible (it is source compatible though). This implies we have to step up to 2.2.0. We have to reenable Mima tests after 2.2.0 release. This will go into a 3.0.0 release, together with React 18 support.

Thanks to @hugo-vrijswijk for the idea of using sourcecode.FullName for the default names.

@@ -13,7 +13,7 @@ object ScalaFnComponentTest extends TestSuite {

final case class Add(x: Int, y: Int)

val CaseClassProps = ScalaFnComponent[Add] { a =>
val CaseClassProps = ScalaFnComponent.withDisplayName("Add")[Add] { a =>

Choose a reason for hiding this comment

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

What happens when not setting displayName? Does it also set the JS facade to ""? And if so, is this different behaviour than leaving it unset?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If not set, it is taken from an implicit sourcecode.FullName.


### New Stuff

- `ScalaFnComponent` and `ScalaForwardRef` now support `.withDisplayName(name)`. Must be called first (before hooks/`render*`/`withChildren*`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something about the default value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea 👍

Copy link
Contributor

@toddburnside toddburnside left a comment

Choose a reason for hiding this comment

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

A lot of work to weave the name through everything, but I don't see a simpler alternative. LGTM.

@rpiaggio
Copy link
Collaborator Author

rpiaggio commented Mar 6, 2024

Since this implementation breaks binary compatibility, it would actually imply a major version bump (ie: 3.0.0).

Seems overkill just for this change, so I'm going to merge this change to our React 18 branch and release this feature there.

@rpiaggio rpiaggio closed this Mar 6, 2024
@rpiaggio rpiaggio reopened this Mar 6, 2024
@rpiaggio rpiaggio changed the base branch from master to topic/react18 March 6, 2024 15:02
@rpiaggio rpiaggio merged commit fdb260b into topic/react18 Mar 6, 2024
2 checks passed
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.

3 participants