-
Notifications
You must be signed in to change notification settings - Fork 324
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: formData bug in react native #1114
base: main
Are you sure you want to change the base?
fix: formData bug in react native #1114
Conversation
chore: lint fix
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/uploadthing/src/internal/upload.browser.ts (1)
71-79
: Introduce a dedicated type interface for the React Native file object
Casting the file object toany
is workable but can weaken type safety. Consider defining a TypeScript interface (e.g.ReactNativeFileObject
) that includesuri
,type
,name
, andrange
to remove theas any
cast, improving clarity and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/uploadthing/src/internal/upload.browser.ts
(1 hunks)
🔇 Additional comments (2)
packages/uploadthing/src/internal/upload.browser.ts (2)
60-70
: Excellent documentation for iOS/React Native edge case
These explanatory comments clarify the underlying crash scenario and the solution approach. Good job documenting the rationale behind the special handling ofFile
metadata for iOS platforms.
80-81
: Fallback logic for standard file upload is intact
Continuing to append a web-basedFile
or slicedFile
preserves legacy behavior and does not introduce any regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/uploadthing/src/_internal/upload-browser.ts (2)
75-85
: Clarify the rationale in code comments.
The documentation is helpful for explaining the React Native iOS crash. However, consider providing a succinct reference link (if available) or a simpler explanation for future maintainers who may not be familiar with iOS’s differences in handling File objects.
86-88
: Add more robust environment checks or type checks.
Currently you rely on the presence of"uri"
to detect React Native. Consider verifying thatfile.type
andfile.name
are also present before building the object, as missing values could lead to unexpected behavior. Likewise, an explicit runtime check fortypeof file.uri === "string"
might improve safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/uploadthing/src/_internal/upload-browser.ts
(1 hunks)
🔇 Additional comments (2)
packages/uploadthing/src/_internal/upload-browser.ts (2)
89-92
: Validate optional properties before appending them.
When adding therange
property conditionally, ensure it is recognized by the server or that the receiving environment properly ignores unknown fields. If not, consider removing or renaming it to avoid potential conflicts in other platforms.
94-96
: Confirm file slicing behavior across environments.
WhenrangeStart > 0
, the code callsfile.slice(...)
. This may not behave consistently in certain non-browser or polyfilled environments. Consider verifying thatfile.slice(...)
is supported wherever this code runs, or provide a fallback if not.
@juliusmarminge hi, kindly reminder |
if ("uri" in file) { | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument | ||
formData.append("file", { | ||
uri: file.uri as string, | ||
type: file.type, | ||
name: file.name, | ||
...(rangeStart > 0 && { range: rangeStart }), | ||
} as any); | ||
} else { | ||
formData.append("file", rangeStart > 0 ? file.slice(rangeStart) : file); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you update the type of the file input to this function so that this is more generally acceptable rather than a big hidden hack?
so like a FormDataFileEsque
type or something on L20 and in cascading places where necessary?
Fix iOS FormData crash in React Native environment
Issue
React Native iOS app crashes when uploading files with error:
attempt to insert nil object from objects[0]
inNSPlaceholderDictionary initWithObjects:forKeys:count
Root Cause
iOS attempts to create NSDictionary from File object when appending to FormData, but fails because it expects specific structure
{uri, type, name}
.Solution
Add environment detection and handle FormData differently for React Native:
uri
property to detect RN environment{uri, type, name}
structureBreaking Changes
None. Maintains backward compatibility with web environments.
Summary by CodeRabbit