Add new private vips package
#64845
Add new private vips package
#64845
Conversation
swissspidy
added
[Type] Enhancement
swissspidy
force-pushed
the
fix/61447-vips-package
branch
3 times, most recently
from
1635cb4
to
7e84afe
Compare
swissspidy
force-pushed
the
fix/61447-vips-package
branch
from
7e84afe
to
541cf92
Compare
|
Size Change: +36.2 kB (+2.05%) Total Size: 1.8 MB
ℹ️ View Unchanged
|
swissspidy
force-pushed
the
fix/61447-vips-package
branch
from
541cf92
to
97de5b2
Compare
tools/webpack/shared.js
Outdated
| resolve: { | ||
| // Ensure "require" has a higher priority when matching export conditions. | ||
| // Needed for wasm-vips which is used by the @wordpress/vips package. | ||
| // https://webpack.js.org/configuration/resolve/#resolveconditionnames | ||
| // https://github.com/kleisauke/wasm-vips/issues/50#issuecomment-1664118137 | ||
| conditionNames: [ 'require', 'import' ], | ||
| }, |
There was a problem hiding this comment.
Looks like this has implications for other packages as well :/
There was a problem hiding this comment.
There's a package patch in https://github.com/WordPress/gutenberg/pull/65250/files?show-viewed-files=true&file-filters%5B%5D=#diff-57cf15f5ef3f76d6b8f1dcfe9ca40e86887fd295f80df3fb2bc0565352520d31 that can be used instead of this change.
|
@youknowriad @ntsekouras Would appreciate your early thoughts on this one The It is a wrapper around There are currently two big issues with that:
The whole thing is best tested once I add the PR for the |
swissspidy
force-pushed
the
fix/61447-vips-package
branch
from
97de5b2
to
53e3547
Compare
swissspidy
force-pushed
the
fix/61447-vips-package
branch
from
53e3547
to
b084207
Compare
packages/vips/package.json
Outdated
| "exports": { | ||
| ".": { | ||
| "default": "./src/index.ts" | ||
| } | ||
| }, |
There was a problem hiding this comment.
This seems new, I don't think any other packages use it. That's not a bad thing and we're considering using exports for building more script modules. CC @gziolo 👀
Since this is unused so far in WordPress packages, I'd prefer not to introduce it here. It can be added to all packages at the same time in the same way when we decide to do that.
I am surprised to see it point to a TypeScript file. I think this should point to a compiled file, probably the same as the module field:
| "exports": { | |
| ".": { | |
| "default": "./src/index.ts" | |
| } | |
| }, | |
| "exports": { | |
| ".": { | |
| "default": "./build-module/index.js" | |
| } | |
| }, |
When the field is defined like this with only a single . export, there's a shorthand form: "exports": "./build-module/index.js".
There was a problem hiding this comment.
This is mostly leftover from testing I think, agree it should be removed for consistency :-)
But my main focus right now is getting the WASM thing to work 😄
|
Will you provide some details about the build issues? In particular how should the behavior be verified. Is a successful
|
As per the PR description, after a build, the I'll see if I can add some simple demo or so to verify the behavior more easily in the browser / editor 👍
I don't know. Is this file also used for creating the
As per #64845 (comment), I need to load the CJS version of See the With the change to |
|
Will you share some details on the unintended consequences for other packages? Is it breaking some behavior or how does it manifest? |
The bundle size increases for |
|
@sirreal I just pushed a bunch of changes to make it easier to test the PR. I will remove those changes again before merge. You can test this by dropping an image file into the editor. It should upload normally, but behind the scenes it will be compressed by vips. The setup is similar to the desired end state with the new client-side media processing. Note: We probably don't want to use |
|
This is a mess 😖 In theory, webpack supports wasm natively by enabling patch with some ideasdiff --git before/patches/wasm-vips+0.0.10.patch after/patches/wasm-vips+0.0.10.patch
new file mode 100644
index 00000000000..3310b706e5a
--- /dev/null
+++ after/patches/wasm-vips+0.0.10.patch
@@ -0,0 +1,15 @@
+diff --git a/node_modules/wasm-vips/package.json b/node_modules/wasm-vips/package.json
+index c20f052..b26a38a 100644
+--- a/node_modules/wasm-vips/package.json
++++ b/node_modules/wasm-vips/package.json
+@@ -15,10 +15,6 @@
+ "type": "commonjs",
+ "exports": {
+ ".": {
+- "browser": {
+- "import": "./lib/vips-es6.js",
+- "require": "./lib/vips.js"
+- },
+ "node": {
+ "import": "./lib/vips-node.mjs",
+ "require": "./lib/vips-node.js"
diff --git before/tools/webpack/packages.js after/tools/webpack/packages.js
index d4bf1243c8f..6fd792e424f 100644
--- before/tools/webpack/packages.js
+++ after/tools/webpack/packages.js
@@ -143,20 +143,6 @@ module.exports = {
// This is the default, but required for @shopify/web-worker.
globalObject: 'self',
},
- module: {
- rules: [
- ...baseConfig.module.rules,
- {
- test: /\.wasm$/,
- type: 'asset/resource',
- generator: {
- // FIXME: Do not hardcode path.
- filename: './build/vips/[name].wasm',
- publicPath: '',
- },
- },
- ],
- },
performance: {
hints: false, // disable warnings about package sizes
},
diff --git before/tools/webpack/shared.js after/tools/webpack/shared.js
index bad55ca53cd..51aba37dee0 100644
--- before/tools/webpack/shared.js
+++ after/tools/webpack/shared.js
@@ -16,6 +16,9 @@ const { NODE_ENV: mode = 'development', WP_DEVTOOL: devtool = 'source-map' } =
const baseConfig = {
target: 'browserslist',
+ experiments: {
+ asyncWebAssembly: true,
+ },
optimization: {
// Only concatenate modules in production, when not analyzing bundles.
concatenateModules:
@@ -48,13 +51,6 @@ const baseConfig = {
},
],
},
- resolve: {
- // Ensure "require" has a higher priority when matching export conditions.
- // Needed for wasm-vips which is used by the @wordpress/vips package.
- // https://webpack.js.org/configuration/resolve/#resolveconditionnames
- // https://github.com/kleisauke/wasm-vips/issues/50#issuecomment-1664118137
- conditionNames: [ 'require', 'import' ],
- },
watchOptions: {
ignored: [
'**/node_modules',Could this be used as a Script Module? I haven't tried yet, but if it were compiled as an actual module it might solve some of these issues. I'm working on building some more script modules in #65064. |
Sort of... At least right now, the PR in its current form, the WASM files were correctly loaded, so it did seem to work as expected.
What exactly isn't working? Does that break the build step? (Aside: the wasm-vips maintainer is super responsive, in case we need to make any upstream changes)
Are script modules really the answer? 😄 That opens a whole other can of worms IMHO. This library needs to be called from the editor, but that isn't a script module. |
|
I plan to help sort out the details here, but I'm prioritizing other work I hope to ship in WordPress 6.7. After that work is settled I can dedicate some more focus to this. Some of that work is related, such as how we're building and shipping Script Modules: #65460
This is always good if we can land the parts that are clear and postpone the parts that are complicated 👍 |
|
New PR for the PHP changes: #65701 |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I removed all the web worker stuff now from this PR to keep this to the bare minimum, unblocking further work on this feature. @sirreal I'd appreciate another review here, just focusing on the existing code. The real fun begins (again) in subsequent PRs when actually using the wasm files in a worker. |
There was a problem hiding this comment.
I've been reviewing this in depth, I'm leaving another round of feedback. I haven't finished reviewing the main TS files yet.
I'd strongly prefer that if and when this arrives in WordPress it's as a script module. I know there are pieces missing for that but they're things I'm working on and I think we can have for 6.8.
On Monday I'll finish this complete review. Thanks for your patience 🙂
There was a problem hiding this comment.
I've reviewed this and it seems ready. There are some remaining questions and minor suggestions from my previous review #64845 (review), but nothing blocking.
| }, | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
Can someone explain to me how we're building this. What's the story here? We just copy the "wasm" files into the built folders? What does webpack outputs when it finds a wasm dependency? What about the npm packages that we publish, are we changing the constraints for developers using these npm packages if these packages start to require wasm dependencies?
Basically, I'm trying to understand the impact on:
- npm consumers.
- WordPress scripts.
There was a problem hiding this comment.
It's configured as a webpack "asset", similar to images and other assets. Webpack basically copies the file and exposes the path when the asset is imported.
It shouldn't be published to npm for now, so I don't think that's a concern at this time.
For WordPress scripts it should work because we can figure out the path that needs to be used to access the wasm assets.
These wasm assets are one reason I'd really like to use script modules for this package. Webpack has some wasm module support and the library relies on some modules features, so there may be some simplification available.
There was a problem hiding this comment.
It shouldn't be published to npm for now, so I don't think that's a concern at this time.
For me that's a concern, my guess is that we're going to add the vips packages as a dependency of block-editor package. So my question is what happens to third-party block editors, will they have to change how their applications builds, feels heavy? How do regular npm packages (in the npm registry) that use wasm dependencies work.
There was a problem hiding this comment.
In other words, will we have to update https://wordpress.org/gutenberg-framework/ as a consequence of this?
There was a problem hiding this comment.
For me that's a concern, my guess is that we're going to add the vips packages as a dependency of block-editor package.
Indirectly through the new upload-media package, yes. Note that first this will be behind an experiment in the plugin only, and wouldn't be exposed through the published npm packages (at least that's my idea). So we have some time to iterate on this. Right now this change doesn't affect any published packages.
So my question is what happens to third-party block editors, will they have to change how their applications builds, feels heavy? How do regular npm packages (in the npm registry) that use wasm dependencies work.
In other words, will we have to update https://wordpress.org/gutenberg-framework/ as a consequence of this?
My understanding is that third-party block editor applications will need to update their bundler config to ensure that it supports wasm files. Once this package is published as a script module only, usage might become a bit easier.
Vite (as used by https://wordpress.org/gutenberg-framework/) has some wasm support built-in, but for ESM integration there is vite-plugin-wasm
There was a problem hiding this comment.
I wonder if asset/inline is actually better for us, that way we can ship npm packages that are ready to use regardless of the bundler config. I'm not sure how I feel about pushing this burden towards consumers.
Ok for me to start as a Gutenberg experiment not exposed to WordPress and npm users.
There was a problem hiding this comment.
I wonder if
asset/inlineis actually better for us, that way we can ship npm packages that are ready to use regardless of the bundler config.
That sounds really bad for performance if we shove several MB worth of WASM files as data URIs into the JS bundles... I'd much rather have them as separate files so that they can be cached by the browser or even offloaded to a CDN.
There was a problem hiding this comment.
That sounds really bad for performance if we shove several MB worth of WASM files as data URIs into the JS bundles... I'd much rather have them as separate files so that they can be cached by the browser or even offloaded to a CDN.
Isn't it the same thing for the JS, it can also be cached and offloaded if needed, not sure I follow what's different here.
- Use `interface` instead of `type` - Clarify `force` option
This comment was marked as resolved.
This comment was marked as resolved.
What?
This is a new package for client-side media processing, see #61447.
It is a wrapper around wasm-vips, which is a WASM port of the powerful vips image library.
This package has been tested very well as part of https://github.com/swissspidy/media-experiments, where it runs in a dedicated web worker.
The package is private and shall not be published until later on when all of the subsequent PRs are merged.
Why?
Required for browser-based image editing.
How?
Adds a new lightweight package for using vips in a browser.
It's a separate package so it can be easily loaded in a (inline) web worker. That's also why there is this
setLocationmethod, to ensure that works in a WordPress context.Testing Instructions
None at this time, as this is not user-facing or anything.
There are some tests which should pass of course.
Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A