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

fix(jsx): allow null, undefined, and boolean to be returned from function component #3241

Merged
merged 3 commits into from
Aug 8, 2024

Conversation

usualoma
Copy link
Member

@usualoma usualoma commented Aug 7, 2024

fixes #3235
The deno converts an empty Fragment to null when the value of the "jsx" entry is "precompile", but "hono/jsx" was not able to handle the function component returning null, which caused the problem #3235.

What should be the returned value of an FC type?

In the current React type definition, FunctionComponent returns the ReactNode, and since ReactNode also includes boolean and undefined, it would be preferable if Hono's FC also included these (boolean and undefined).

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L1114-L1122
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L478-L489

However, the changes to add undefined and boolean have a considerable impact on existing code, so I have chosen not to do so in this pull request. I would like to discuss this at another time.

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code

@usualoma
Copy link
Member Author

usualoma commented Aug 7, 2024

@yusukebe Would you please review?

Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.21%. Comparing base (e123546) to head (1a6d822).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3241   +/-   ##
=======================================
  Coverage   96.21%   96.21%           
=======================================
  Files         151      151           
  Lines       15102    15105    +3     
  Branches     2660     2664    +4     
=======================================
+ Hits        14530    14533    +3     
  Misses        572      572           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yusukebe
Copy link
Member

yusukebe commented Aug 7, 2024

@usualoma

Okay! I'll do it later.

@yusukebe
Copy link
Member

yusukebe commented Aug 7, 2024

Hi @usualoma

Looks good. I want to know:

In the current React type definition, FunctionComponent returns the ReactNode, and since ReactNode also includes boolean and undefined, it would be preferable if Hono's FC also included these (boolean and undefined).

Does this issue already exist before this PR (Or #3235)? As you said, I also think we should not include the change in this PR.

@usualoma
Copy link
Member Author

usualoma commented Aug 7, 2024

@yusukebe

Thanks for the review!

The FC return type divergence was noticed during this work. I don't think there are any known issues.

I think it would be better to include undefined and boolean, but I think components that return undefined or boolean are very rare. Changing it would likely affect the result of <Component/>.toString(), so I don't think it should be changed unless there is some special motivation.

@yusukebe
Copy link
Member

yusukebe commented Aug 8, 2024

@usualoma

Thank you for your reply.

I think it would be better to include undefined and boolean, but I think components that return undefined or boolean are very rare. Changing it would likely affect the result of <Component/>.toString(), so I don't think it should be changed unless there is some special motivation.

I agree 👍

Now, let's merge this one!

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@yusukebe yusukebe merged commit fbed2df into honojs:main Aug 8, 2024
14 checks passed
adamnolte referenced this pull request in autoblocksai/cli Aug 12, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [hono](https://hono.dev/) ([source](https://togithub.com/honojs/hono))
| [`4.5.4` ->
`4.5.5`](https://renovatebot.com/diffs/npm/hono/4.5.4/4.5.5) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/hono/4.5.5?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/hono/4.5.5?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/hono/4.5.4/4.5.5?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/hono/4.5.4/4.5.5?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>honojs/hono (hono)</summary>

### [`v4.5.5`](https://togithub.com/honojs/hono/releases/tag/v4.5.5)

[Compare
Source](https://togithub.com/honojs/hono/compare/v4.5.4...v4.5.5)

#### What's Changed

- fix(jsx): allow null, undefined, and boolean to be returned from
function component by [@&#8203;usualoma](https://togithub.com/usualoma)
in
[https://github.com/honojs/hono/pull/3241](https://togithub.com/honojs/hono/pull/3241)
- feat(context): Add types for `c.header` by
[@&#8203;nakasyou](https://togithub.com/nakasyou) in
[https://github.com/honojs/hono/pull/3221](https://togithub.com/honojs/hono/pull/3221)
- fix(jsx): fix draggable type to accept boolean by
[@&#8203;yasuaki640](https://togithub.com/yasuaki640) in
[https://github.com/honojs/hono/pull/3253](https://togithub.com/honojs/hono/pull/3253)
- feat(context): add Context-Type types to `c.header` by
[@&#8203;nakasyou](https://togithub.com/nakasyou) in
[https://github.com/honojs/hono/pull/3255](https://togithub.com/honojs/hono/pull/3255)
- fix(serve-static): supports directory contains `.` and not end `/` by
[@&#8203;yusukebe](https://togithub.com/yusukebe) in
[https://github.com/honojs/hono/pull/3256](https://togithub.com/honojs/hono/pull/3256)

**Full Changelog**:
honojs/hono@v4.5.4...v4.5.5

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 4am on Monday" in timezone
America/Chicago, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job log](https://developer.mend.io/github/autoblocksai/cli).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yMC4xIiwidXBkYXRlZEluVmVyIjoiMzguMjAuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

Fragment does'n work in custom JSX Component
2 participants