Skip to content

fix: treat wrap={undefined} like an omitted wrap prop - #3151

Open
EvHaus wants to merge 1 commit into
diegomura:masterfrom
EvHaus:fix-undefined-wrap
Open

fix: treat wrap={undefined} like an omitted wrap prop#3151
EvHaus wants to merge 1 commit into
diegomura:masterfrom
EvHaus:fix-undefined-wrap

Conversation

@EvHaus

@EvHaus EvHaus commented Apr 19, 2025

Copy link
Copy Markdown
Contributor

Summary

Similar to #3149 and #3142, this fixes a regression from the layout TypeScript conversion.

After that change, an explicit wrap={undefined} was treated as falsy (no wrapping), while an omitted wrap prop still defaulted to true. Those should behave the same.

Behaviour

Usage Result
<View /> true
<View wrap={undefined} /> true
<View wrap={null} /> false
<View wrap={false} /> false
<View wrap={true} /> true

null stays falsy (disables wrapping). Only undefined / omitted use the default.

Also adds unit tests for getWrap.

Test plan

  • yarn vitest run packages/layout/tests/node/getWrap.test.ts
  • Confirm wrapping still works for views with no wrap prop
  • Confirm wrap={false} still prevents page breaks inside the view

@changeset-bot

changeset-bot Bot commented Apr 19, 2025

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 276b6bf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@react-pdf/layout Patch
@react-pdf/renderer Patch
@react-pdf/math Patch
@react-pdf/mermaid Patch
next-14 Patch
next-15 Patch
@react-pdf/vite-example Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@diegomura

Copy link
Copy Markdown
Owner

Is it correct that undefined was already returning true? I'm not sure about null though. That's a falsy value. Makes more sense to me to false for null than true

@EvHaus

EvHaus commented Sep 30, 2025

Copy link
Copy Markdown
Contributor Author

Is it correct that undefined was already returning true?

The current behaviour is:

  1. <View /> is treated as true
  2. <View wrap={undefined} /> is treated as false
  3. <View wrap={null} /> is treated as false
  4. <View wrap={false} /> is treated as false
  5. <View wrap={true} /> is treated as true

I think 1, 4 and 5 make sense. But 2 and 3 are unexpected to me.

I'm not sure about null though. That's a falsy value. Makes more sense to me to false for null than true

I see where you're coming from however given that "no value" (case 1) and undefined (case 2) will evaluate to true I think having null evaluate to true is a more developer-friendly/consistent experience.

Let me know if you feel strongly about it and I'll make the PR change accordingly. Thanks @diegomura!

@diegomura

Copy link
Copy Markdown
Owner

is treated as false

I don't think this is right. This case is treated as true. I believe that's a typo?

But 2 and 3 are unexpected to me.

null and undefined are falsy values. It doesn't make sense to me to be treated as true.

@EvHaus EvHaus changed the title fix: undefined and null wrap prop values are rendered as undefined instead of true by default fix: treat wrap={undefined} like an omitted wrap prop Aug 11, 2026
@EvHaus
EvHaus force-pushed the fix-undefined-wrap branch from 7c8c80d to 276b6bf Compare August 11, 2026 06:13
@EvHaus

EvHaus commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

My bad on the typo. Updated comment and PR description. Kept null handling as falsy. However, I would still argue undefined should be handled as true. Otherwise you end up with:

  • <View /> treated as true
  • <View wrap={undefined} /> treated as false

Which I think is really unexpected/clumbsy behaviour. If you still disagree, feel free to close the PR.

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