upgrade: upload package upgrade for Solid 2.0#846
Open
davedbase wants to merge 8 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: cb3962a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://ghub.bbstale.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/upload/dev/index.tsx (1)
89-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDemo is still using pre-migration picker calls.
Line 9, Line 10, and Line 47 still use
createFileUploaderas a picker (selectFilesflow). In this migration, picker behavior moved tocreateFilePicker, so this example is out of sync and can fail typing/usage validation.Suggested fix
-import { createDropzone, createFileUploader, fileUploader } from "../src/index.js"; +import { createDropzone, createFilePicker, fileUploader } from "../src/index.js"; -const { files, selectFiles } = createFileUploader(); -const { files: filesAsync, selectFiles: selectFilesAsync } = createFileUploader(); +const { files, selectFiles } = createFilePicker(); +const { files: filesAsync, selectFiles: selectFilesAsync } = createFilePicker(); -const { files, selectFiles } = createFileUploader({ +const { files, selectFiles } = createFilePicker({ multiple: true, accept: "image/*", });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/upload/dev/index.tsx` around lines 89 - 103, The demo still uses the old picker API (createFileUploader/selectFiles) causing type/usage mismatches; update any picker usage to the new API by replacing calls to createFileUploader with createFilePicker and adapt the directive/handler (e.g., fileUploader) to call the createFilePicker/selectFiles flow and its new config keys (rename any selectFiles-related callbacks or props to the createFilePicker equivalents) so the ref={fileUploader(...)} invocation and the setFiles/userCallback wiring match the new createFilePicker signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/upload-solid-2-migration.md:
- Around line 5-10: Update the stale migration notes that reference Solid 2.0
beta.7 to match the shipped beta.10: change the header/version string "Solid.js
2.0 (beta.7)" and the two bullet items that mention `solid-js@^2.0.0-beta.7` and
`@solidjs/web@^2.0.0-beta.7` to `beta.10`, and verify the descriptions for
`isServer`, `createDropzone`, and `fileUploader` remain accurate for the beta.10
API surface so the migration guidance aligns with the published package
metadata.
In `@packages/upload/src/createDropzone.ts`:
- Around line 62-98: The isDragging state should be driven by drop-target
events, not source events: update the handlers so setIsDragging(true) is called
in onDragEnter and/or onDragOver, and setIsDragging(false) is called in
onDragLeave and onDrop (remove or stop relying on setIsDragging in
onDragStart/onDragEnd); locate the handlers named onDragStart, onDragEnd,
onDragEnter, onDragLeave, onDragOver and onDrop and move/adjust the
setIsDragging calls accordingly so external OS/file drags correctly toggle the
dragging UI.
In `@packages/upload/src/createFileUploader.ts`:
- Around line 69-99: The code calls the user-supplied send(...) directly inside
uploadFiles.map so a synchronous throw escapes the per-file rejection handler;
wrap the send call so sync throws are converted into a rejected Promise and
handled the same way as async rejections. Specifically, around the send(...)
invocation used in uploadFiles.map (which currently passes the progress callback
and controllers[i]!.signal and then chains .then(...) handlers), ensure you call
Promise.resolve().then(() => send(...)) or use a try/catch that returns
Promise.reject(err) so any synchronous exception flows into the existing
.then(success, error) branch and the setFiles updates (status, response, error)
for the corresponding index i still run when generation === thisGen.
In `@packages/upload/src/fileSender.ts`:
- Around line 58-61: The fileSender currently forwards all headers to
xhr.setRequestHeader which allows callers to override Content-Type and break
FormData multipart boundaries; update the header loop in fileSender so that if
the payload is a FormData (check via data instanceof FormData) you skip any
header whose name (case-insensitively) equals "content-type" and still set all
other headers via xhr.setRequestHeader; ensure the check uses the existing
headers variable and xhr so callers’ other headers remain intact while letting
the browser supply the correct multipart Content-Type.
In `@packages/upload/src/fileUploader.ts`:
- Around line 20-33: The onChange handler (function onChange) fails to reset the
file input value, so selecting the same file won't retrigger change events;
after calling transformFiles and before/after invoking userCallback (and
regardless of success or error), clear the input by setting target.value = ''
(or target.value = null) to reset the file input; ensure this happens even when
userCallback throws by putting the value-reset in a finally block or after the
try/catch so setFiles(parsedFiles), await userCallback(parsedFiles), the onError
handling, and then resetting target.value are executed in all paths.
In `@packages/upload/src/types.ts`:
- Around line 29-30: Change the removal API to use a stable unique identifier
instead of fileName: update the removeFile signature in types (and any matching
interfaces) from removeFile(fileName: string) => void to removeFile(fileId:
string) => void (or a typed FileId/handle), then update all
callers/implementations (createFilePicker, createDropzone, createFileUploader
and any other code referenced at the same locations) to assign and propagate a
stable id for each added file and perform removal by that id so duplicates are
disambiguated; ensure exported types and internal logic consistently use the new
id field and adapt tests/consumers accordingly.
In `@packages/upload/test/index.test.tsx`:
- Around line 397-401: The test creates a Solid root and immediately calls
dispose() before invoking the ref callback, which hides lifecycle/owner issues;
update the tests (instances using createRoot and the ref variable produced by
fileUploader) to call ref(input) while the root is still active and only call
dispose() afterwards—i.e., move the dispose() call to after invoking ref (and
any assertions) for the fileUploader ref usages at the shown locations so the
ref callback runs inside the active root and cleanup happens afterwards.
- Around line 804-817: The test starts a reactive root via createRoot and calls
upload(...) but never settles the mocked XHR nor disposes the root, causing
leaked state across tests; update the test that uses
makeMockXhr/createRoot/upload/makeUploadFile/fileSender to (1) resolve the
mocked XHR response (e.g., call the mock XHR's success handler or resolver such
as xhr.onload/xhr.resolve/xhr.respond) so the upload promise settles and then
await the upload call, and (2) call the returned dispose() (or use
upload(...).finally(() => dispose())) to tear down the reactive root; apply the
same change to the other test mentioned.
---
Outside diff comments:
In `@packages/upload/dev/index.tsx`:
- Around line 89-103: The demo still uses the old picker API
(createFileUploader/selectFiles) causing type/usage mismatches; update any
picker usage to the new API by replacing calls to createFileUploader with
createFilePicker and adapt the directive/handler (e.g., fileUploader) to call
the createFilePicker/selectFiles flow and its new config keys (rename any
selectFiles-related callbacks or props to the createFilePicker equivalents) so
the ref={fileUploader(...)} invocation and the setFiles/userCallback wiring
match the new createFilePicker signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 17ecaa19-43b8-4b8c-8045-507f1f1c147d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.changeset/upload-solid-2-migration.mdpackages/upload/README.mdpackages/upload/dev/index.tsxpackages/upload/package.jsonpackages/upload/src/createDropzone.tspackages/upload/src/createFilePicker.tspackages/upload/src/createFileUploader.tspackages/upload/src/fileSender.tspackages/upload/src/fileUploader.tspackages/upload/src/index.tspackages/upload/src/types.tspackages/upload/test/index.test.tsxpackages/upload/tsconfig.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@solid-primitives/uploadtargeting Solid.js 2.0 (solid-js@^2.0.0-beta.10,@solidjs/web@^2.0.0-beta.10)createFileUpload→createFileUploaderfor naming consistency with the package@solid-primitives/utilsdependency with inline equivalents (the utils package is not yet on Solid 2.0)0to reflect the breaking API changesNew:
createFileUploaderReplaces the old single-request
createFileUpload. Key differences:progress,status,error, andresponseare tracked in acreateStorearray (files: readonly FileUploadEntry[]) for fine-grained JSX reactivity — only the changed row re-rendersprogressandstatusarecreateMemoderivations across all entries (uploading > error > aborted > success > idle)SendFunctionis passed in explicitly;fileSender(XHR) is a separate named export so bundlers can drop it when unusedremoveFile(fileName)andclearFiles()— remove entries from the store; both abort all in-flight uploads and bump a generation counter so stale async closures abandon their store writesAbortController;abort()cancels all;upload()called again cancels the previous batchcreateFilePickercreateFileUploader(which was a misnomer — it only selects files)isLoadingaccessor:truewhile theselectFilescallback is pendingremoveFileandclearFilesJSX.EventHandlerusage (removed fromsolid-jsin 2.0) — handler now typed as(event: Event) => voidcreateDropzoneJSX.EventHandler<T, DragEvent>(no longer insolid-js) — all 7 handlers retyped as(event: DragEvent) => void, removing 14as anycastsisServerimport updated to@solidjs/webfileUploaderuse:directive)Summary by CodeRabbit
Release Notes –
@solid-primitives/uploadv0.2.0New Features
createFilePicker()for native file selection with reactive state (files,error,isLoading)fileSender()factory for XHR-based file uploads with progress trackingcreateDropzonenow exposes reactiveerrorandisLoadingstateBreaking Changes
createFileUploadernow requires an explicitSendFunctiontransport parameterfileUploaderis now a ref callback (ref={fileUploader(opts)}) instead of a directive