nksaraf / vinxi

The Full Stack JavaScript SDK
https://vinxi.vercel.app
MIT License
1.84k stars 74 forks source link

[Issue][unplugin-imagemin] Plugin is not working #108

Open qlaffont opened 8 months ago

qlaffont commented 8 months ago

ref : https://github.com/nksaraf/vinxi/issues/44 repro : https://github.com/qlaffont/test-cmp



> test-cmp@ build /home/quentin/Documents/dev/test/test-cmp
> vinxi build

vinxi 0.0.60
start {
  appRoot: './src',
  ssr: false,
  islands: false,
  server: { base: undefined },
  solid: {}
}
vinxi Found vite.config.js with app config

┌────────────────────────────┐
│ ⚙  Building your app...    │
└────────────────────────────┘
removing /home/quentin/Documents/dev/test/test-cmp/.vinxi/build/ssr
removing /home/quentin/Documents/dev/test/test-cmp/.vinxi/build/client
removing /home/quentin/Documents/dev/test/test-cmp/.vinxi/build/server-fns

┌───────────────────────────────┐
│ 📦 Compiling ssr router...    │
└───────────────────────────────┘
vinxi building router ssr in handler mode
vite v4.5.0 building SSR bundle for production...
"createHandler" is imported from external module "h3" but never used in "node_modules/.pnpm/vinxi@0.0.60_rollup@3.29.4_terser@5.26.0/node_modules/vinxi/runtime/server.js".
✓ 40 modules transformed.
.vinxi/build/ssr/manifest.json                   1.40 kB
.vinxi/build/ssr/assets/index-fb87c914.css      46.63 kB
.vinxi/build/ssr/_...404_.js                     0.74 kB
.vinxi/build/ssr/about.js                        0.96 kB
.vinxi/build/ssr/assets/components-86ea9901.js   1.20 kB
.vinxi/build/ssr/index.js                        3.61 kB
.vinxi/build/ssr/ssr.js                          6.50 kB

[unplugin-imagemin] 📦 📦 Process start with Mode sharp
✓ built in 743ms
✔ build done                                                                                                                                                  7:01:06 AM

┌──────────────────────────────────┐
│ 📦 Compiling client router...    │
└──────────────────────────────────┘
vinxi building router client in build mode
[
  '#vinxi/handler/client',
  '/home/quentin/Documents/dev/test/test-cmp/src/routes/[...404].tsx?pick=default&pick=$css',
  '/home/quentin/Documents/dev/test/test-cmp/src/routes/about.tsx?pick=default&pick=$css',
  '/home/quentin/Documents/dev/test/test-cmp/src/routes/index.tsx?pick=default&pick=$css'
]
plugin-legacy overrode 'build.target'. You should pass 'targets' as an option to this plugin with the list of legacy browsers to support instead.
vite v4.5.0 building for production...
virtual:#vinxi/handler/client (1:106) "default" is not exported by "src/entry-client.tsx", imported by "virtual:#vinxi/handler/client".
✓ 115 modules transformed.
node:internal/process/promises:288
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[Error: ENOENT: no such file or directory, open '/home/quentin/Documents/dev/test/test-cmp/.vinxi/build/client/_build/assets/_...404_-legacy-ca63b2ec.js'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/home/quentin/Documents/dev/test/test-cmp/.vinxi/build/client/_build/assets/_...404_-legacy-ca63b2ec.js'
}

Node.js v18.17.1
 ELIFECYCLE  Command failed with exit code 1.```
 
nksaraf commented 8 months ago

Will take a look!

qlaffont commented 8 months ago

@nksaraf any update ?

qlaffont commented 8 months ago

@nksaraf any update ?

nksaraf commented 8 months ago

Haven't been able to give this one time. It could take me a bit longer. @edivados do you think you could help out on this one?

edivados commented 8 months ago

[!NOTE]
The following is just what I observed. I have not invested a lot of time into this so some things may not be accurate.

  • The image in the example is small and vite will inline it so the plugin doesn't pick it up.
  • beforeBundle: true seems to work.

In vinxi public files are not copied by vite during build and the plugin seems to have two issues when publicDir is set to false.

1. outDir does not exist when generateBundle hook runs

When publicDir is set to false the outDir doesn't exist yet and the write here fails.

https://github.com/unplugin/unplugin-imagemin/blob/d0ec2cbf5fd4d312452507f96d5d1a53781f119c/src/core/context.ts#L644

I don't know much about vite/unplugin so I am not sure if writing to disk here is just wrong or it should not rely on the outDir to exist.

2. Plugin relies on publicDir to construct paths?

When publicDir is false it receives/uses an empty string and the paths it builds here don't exist.

https://github.com/unplugin/unplugin-imagemin/blob/d0ec2cbf5fd4d312452507f96d5d1a53781f119c/src/core/sharp.ts#L62-L64

3. Reusing the same plugin instance between routers

This one is a Solid-Start issue. In Solid-Start when you pass a plugins array it will be assigned to multiple routers which means the same plugin instance is run multiple times.

This plugin has internal state that doesn't get cleaned up leading to the same assets being processed multiple times concurrently causing issues.

A user can get around this problem by using a function instead of an array to set plugins.

{
  plugins: () => [plugin1, plugin2]
}
nksaraf commented 8 months ago

Okay so public dir needs to be set to true for this plugin to work. Can we set it in userland for when this plugin is used. Or we dedupe all assets server and client and public and that way we get all the assets

edivados commented 8 months ago

@nksaraf Played around a bit more with it and updated my previous message. See point 3.

Can we set it in userland for when this plugin is used.

A user could do that. I did this while testing. Not sure what other issues will pop up. Keeping the public router would mean in this case that unprocessed files are being copied to the output.

Turns out removing the public router has no effect because nitro adds an entry to publicAssets for every scanDirs entry so files from the public directory get copied anyway. Nitro adds srcDir to scanDirs so it's not empty. https://github.com/unjs/nitro/blob/7d3ca94f14aa712eaeb0c4f0b8d81b126f2e913b/src/options.ts#L260-L261 https://github.com/unjs/nitro/blob/7d3ca94f14aa712eaeb0c4f0b8d81b126f2e913b/src/nitro.ts#L70-L78

In my opinion this plugin has two things that should be fixed.

Or we dedupe all assets server and client and public and that way we get all the assets

Yeah... in Solid-Start there are duplicates between ssr and client router output. If the plugin didn't have the mentioned issues I could also imagine the static router being a vite build too and making something like this eventually possible.

[
    {
        name: "public",
        mode: "static",
        dir: "./public",
        base: "/",
        plugins: [ imagemin ] // no static assets but public files
      },

      {
        name: "client",
        plugins: [ imagemin ] // no public files but static assets
      }
]
qlaffont commented 8 months ago

So what I need to do or did I need to create issue (and where) ? 😅

edivados commented 8 months ago

@qlaffont It's time to get creative for a short term solution. 🤣 This is what I could come up with. I am making use of the fact that the plugin seems to process assets but not public files with beforeBundle: true and later manually use the plugin to process the public files.

Update: Does not work

Does not work ```ts import {defineConfig} from "@solidjs/start/config"; import legacy from "@vitejs/plugin-legacy"; import devtools from "solid-devtools/vite"; import imagemin from "unplugin-imagemin/vite"; import { cpSync, rmSync, mkdirSync } from "fs"; import { join } from "vinxi/lib/path"; function createImageminPlugin(beforeBundle) { return imagemin({ mode: 'sharp', beforeBundle, // Default configuration options for compressing different pictures conversion: [ { from: 'jpeg', to: 'webp' } ], compress: { jpg: { quality: 50, }, jpeg: { quality: 10, }, png: { quality: 70, }, webp: { quality: 70, }, }, }); } const app = defineConfig({ start: { ssr: true }, plugins: () => [ legacy({ targets: ["defaults","not IE 11"], }), devtools({ /* features options - all disabled by default */ autoname: true, // e.g. enable autoname locator: true, }), createImageminPlugin(true), ], }); app.addRouter({ mode: "static", name: "solid-start-cmp", dir: "node_modules/solid-start-cmp/public", base: "/", }); let publicFilesHandler; app.hooks.hook("app:build:nitro:assets:copy:start", ({ nitro }) => { // temporarily remove public files handler to prevent nitro from copying files. publicFilesHandler = nitro.options.publicAssets.shift(); }); app.hooks.hook("app:build:nitro:assets:copy:end", async ({ nitro }) => { // add public files handler back to the top nitro.options.publicAssets.unshift(publicFilesHandler); const publicBuildDir = join(nitro.options.buildDir, "public"); rmSync(publicBuildDir, { recursive: true, force: true }); mkdirSync(publicBuildDir, { recursive: true }); cpSync("public", publicBuildDir, { recursive: true, force: true }); const plugin = createImageminPlugin(false); await plugin.configResolved({ publicDir: "public", build: { assetsDir: "assets", outDir: nitro.options.buildDir, } }); await plugin.closeBundle.handler(); cpSync(publicBuildDir, nitro.options.output.publicDir, { recursive: true, force: true }); }) export default app; ```
qlaffont commented 8 months ago

Thanks @edivados ! I will try to use it. I will link this issue to the other issue : https://github.com/unplugin/unplugin-imagemin/issues/100 @ErKeLost :wave:

edivados commented 8 months ago

Oh interessting I will have to read through it. Maybe using beforeBundle: true doesn't cover some things.

ErKeLost commented 8 months ago

The resources loaded by src in beforeBundle cannot be parsed as a module. I haven't perfected this part yet. I'm sorry I don't have any extra time to maintain this package because of my work.

edivados commented 8 months ago

Ok I see. Thank you.

nksaraf commented 8 months ago

Can we just fork it and patch it for now and release it under @vinxi namespace .. this image optimizations stuff is probably important enough

qlaffont commented 8 months ago

I think it will be perfect ! We need to see with @ErKeLost

ErKeLost commented 8 months ago

hi

I have time to reply to this question now. Thank you for all your discussions and comments.

Let me answer some of the questions mentioned above.

  1. Picture src is introduced rather than as a module.

In vite, we usually use the following ways to introduce image resources.

  1. Import img from'. / a.png'. // Will be loaded as a module

  2. < img: src= ". / a.png" / >. // It is not usually loaded as a module.

  3. In css file. // Will not be loaded as a module

body {.
    Background-image: url (". / a.png").
}.

Usually we use these ways to load our image resources.

But in fact, the second and third are not loaded by vite as modules, which leads to normal, for example, if we introduce it in a html now, it will not be built as a module in load or other hooks.

But in the second case, we are usually treated as a module by the plug-in so that vite can load it, such as the vue plug-in.

In the third case, the css file will not be treated as a module.

That's why I need to distinguish between beforeBundle and then replace the whole css with rules after the last rendersource to achieve the effect.

At present, the solid plugin look like does not implement the second way to load the module,(I haven't had time to check the implementation of solid yet.) so the situation of img src is not loaded as a module.

Due to the complexity of the current project caused by beforeBundle and squoosh and sharp svgo, and the confusion of the code, I recently have time to ReFactor the logic of the entire render, and I will also see if other implementations have a way to load all possible ways to load img as a module.

Thank you again for your reply.

ErKeLost commented 8 months ago

I think it will be perfect ! We need to see with 我认为这将是完美的!我们需要看看@ErKeLost

This looks great. I will study whether the current problem can be solved perfectly.