Add/client side media uploads try local vips no shopify package#27
Draft
adamsilverstein wants to merge 9 commits into
Draft
Conversation
- Introduced a new TypeScript declaration file for importing WASM and VIPS JS files as Base64 data URLs. - Updated the TypeScript configuration to exclude the lib directory. - Created a custom Webpack loader to convert WASM and JS files to Base64 data URLs, facilitating inlining of binary files. - Modified Webpack configuration to use the new loader for handling .wasm and VIPS JS files.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
…ia-uploads-try-local-vips-no-shopify-package
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements client-side media uploads using local VIPS files instead of CDN dependencies, removing the deprecated @shopify/web-worker package. The changes aim to avoid server configuration issues and CORS problems by inlining VIPS library files as Base64 data URLs.
Key changes:
- Replaces the deprecated
@shopify/web-workerpackage with a native Web Worker implementation - Converts VIPS library files (WASM and JS) to Base64 data URLs for local embedding
- Implements a custom webpack loader for handling VIPS files
Reviewed Changes
Copilot reviewed 12 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/webpack/packages.js | Updates webpack rules to use custom WASM loader for VIPS files |
| packages/vips/wasm-loader.js | Custom webpack loader that converts WASM/JS files to Base64 data URLs |
| packages/vips/generate-lib-data.js | Build script that generates TypeScript file with Base64-encoded VIPS libraries |
| packages/vips/src/index.ts | Updates VIPS initialization to use local Base64 files with CDN fallback |
| packages/upload-media/src/utils/worker-factory.ts | Native Web Worker implementation replacing @shopify/web-worker |
| packages/upload-media/src/store/utils/vips.ts | Updates to use new native worker factory |
| Multiple package.json files | Removes @shopify/web-worker dependency |
Comments suppressed due to low confidence (2)
packages/vips/src/index.ts:80
- The version 0.0.14 appears to be incorrect. The wasm-vips package uses semantic versioning and the latest stable version is likely in the 0.0.x range, but 0.0.14 may not exist. Consider using a known stable version or 'latest'.
'https://cdn.jsdelivr.net/npm/wasm-vips@0.0.14/lib';
packages/vips/generate-lib-data.js:34
- [nitpick] The error message could be more helpful by suggesting specific installation commands or indicating which package needs to be installed (e.g., 'npm install wasm-vips').
);
| const resourcePath = this.resourcePath; | ||
|
|
||
| // Debug: log when loader is called | ||
| console.log( 'WASM Loader called for:', resourcePath ); |
There was a problem hiding this comment.
Debug console.log statements should be removed from production code. Consider using a proper logging mechanism or removing this line entirely.
Suggested change
| console.log( 'WASM Loader called for:', resourcePath ); | |
| // console.log( 'WASM Loader called for:', resourcePath ); |
Comment on lines
+27
to
+34
| * Handle messages from the main thread | ||
| */ | ||
| self.addEventListener('message', async (event) => { | ||
| const { id, method, args } = event.data; | ||
|
|
||
| try { | ||
| // Dynamically import the VIPS module | ||
| const vipsModule = await import('@wordpress/vips'); |
There was a problem hiding this comment.
Dynamic imports inside a Web Worker created from a blob URL may not resolve module paths correctly. The '@wordpress/vips' module path might not be available in the worker context, potentially causing runtime errors.
Suggested change
| * Handle messages from the main thread | |
| */ | |
| self.addEventListener('message', async (event) => { | |
| const { id, method, args } = event.data; | |
| try { | |
| // Dynamically import the VIPS module | |
| const vipsModule = await import('@wordpress/vips'); | |
| * Initialize worker context with passed module | |
| */ | |
| self.vipsModule = null; | |
| self.addEventListener('message', async (event) => { | |
| if (event.data.type === 'initialize') { | |
| self.vipsModule = event.data.module; | |
| return; | |
| } | |
| const { id, method, args } = event.data; | |
| try { | |
| // Dynamically import the VIPS module | |
| const vipsModule = self.vipsModule; |
| const workerUrl = URL.createObjectURL( blob ); | ||
|
|
||
| // Create the actual worker | ||
| const worker = new Worker( workerUrl, { type: 'module' } ); |
There was a problem hiding this comment.
Creating a worker with type: 'module' requires the worker script to use ES modules, but the inline worker code uses CommonJS-style imports. This will likely cause the worker to fail to load.
|
Size Change: +2.68 MB (+138.11%) 🆘 Total Size: 4.63 MB
ℹ️ View Unchanged
|
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
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.
What?
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast