vitejs / vite

Next generation frontend tooling. It's fast!
http://vitejs.dev
MIT License
67.08k stars 6.02k forks source link

Vanilla JS instanceof check fails if using pnpm as packagemanager and preserveSymlinks set to true in vite.config.ts #17631

Closed msprada closed 1 month ago

msprada commented 2 months ago

Describe the bug

Before I created this issue here, I had experienced an issue with a svelte kit application which uses vite under the hood. This issue can be found here Original Svelte Kit Issue or the reduced one Part II. First I thought the issue related to the svelte kit framework but in the meantime I'm no longer convinced.

I created a small repo with a defaulft vanilla installation of ts and vite.

Update 07/09/2024: As mentioned by hi-ogawa I did not manage to create a vanilla ts example which mirrors the exact issue I would guess. So we should consider the Original Issue Part II. I also include the explanation within follwing comment https://github.com/vitejs/vite/issues/17631#issuecomment-2217244611.

The svelte kit application uses a default javascript api "instanceof" to check whether an thrown error is a Redirect Class or not. If a redirect is detected the application redirects the user.

If the check fails the user will not be redirected.

There is a direct influence of this behavour in regard to the vite.config.ts setting preserveSymlinks.

If "preserveSymlinks" is set to true the instanceof check will fail and the user is not redirected at all.

If "preserveSymlinks" is set to false the user is redirected.

Reproduction

https://github.com/msprada/vite-pnpm-issue-preserveSymlinks https://github.com/msprada/svelte-kit-issue--12139-II

Steps to reproduce

To Reproduce

Be aware

System Info

System:
    OS: macOS 13.6.7
    CPU: (8) x64 Intel(R) Core(TM) i7-4850HQ CPU @ 2.30GHz
    Memory: 1.79 GB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 22.2.0 - ~/.nvm/versions/node/v22.2.0/bin/node
    npm: 10.7.0 - ~/.nvm/versions/node/v22.2.0/bin/npm
    pnpm: 9.2.0 - /usr/local/bin/pnpm
  Browsers:
    Brave Browser: 126.1.67.123
    Safari: 16.6
  npmPackages:
    vite: ^5.3.1 => 5.3.3

Used Package Manager

pnpm

Logs

No response

Validations

msprada commented 2 months ago

I wante to create a post on your discord server first before writing the issue her but: I joined the vite land discord server. I wanted to create a post in channel "community" under help but I do not have the permission for this.

image

My User Name is: msprada.

Maybe someone can give me the permission.

Thanks a lot.

hi-ogawa commented 2 months ago

I think this is working as intended. preserveSymlinks means that when importing a "same" module from a real path and symlink pat, the module won't be de-duplicated (in this case, this leads to two distinct Redirect classes).

The doc from esbuild explains it better https://esbuild.github.io/api/#preserve-symlinks

Keep in mind that this means a file may be given multiple identities if there are multiple symlinks pointing to it, which can result in it appearing multiple times in generated output files.

msprada commented 1 month ago

I think this is working as intended. preserveSymlinks means that when importing a "same" module from a real path and symlink pat, the module won't be de-duplicated (in this case, this leads to two distinct Redirect classes).

The doc from esbuild explains it better https://esbuild.github.io/api/#preserve-symlinks

Keep in mind that this means a file may be given multiple identities if there are multiple symlinks pointing to it, which can result in it appearing multiple times in generated output files.

Maybe the vanilla example which I try to recreate is not specific enough. I'll try to clarify this in more detail and I'll use this issue in svelte. The structure of the solution and the imports are different.

There is a server component which implements the redirect() function which is imported in following way:

import {redirect } from '@sveltejs/kit'; which points to file => my-app/nodemodules/.pnpm/@sveltejs+kit@2.5.18@sveltejs+vite-plugin-svelte@3.1.1_svelte@4.2.18_vite@5.3.3__svelte@4.2.18_vite@5.3.3/node_modules/@sveltejs/kit/src/exports/index.js

in this function a class Redirect is thrown

export function redirect(status, location) {
    if ((!BROWSER || DEV) && (isNaN(status) || status < 300 || status > 308)) {
        throw new Error('Invalid status code');
    }

    throw new Redirect(
        // @ts-ignore
        status,
        location.toString()
    );
}

The Redirect is imported with following import: import { HttpError, Redirect, ActionFailure } from '../runtime/control.js';

which points to file => "my-app/nodemodules/.pnpm/@sveltejs+kit@2.5.18@sveltejs+vite-plugin-svelte@3.1.1_svelte@4.2.18_vite@5.3.3__svelte@4.2.18_vite@5.3.3/node_modules/@sveltejs/kit/src/runtime/control"

The instance check which fails is located in following file: _my-app/nodemodules/.pnpm/@sveltejs+kit@2.5.18@sveltejs+vite-plugin-svelte@3.1.1_svelte@4.2.18_vite@5.3.3__svelte@4.2.18_vite@5.3.3/nodemodules/@sveltejs/kit/src/runtime/server/page/index.js line 208

if (err instanceof Redirect) {
                        if (state.prerendering && should_prerender_data) {
                            const body = JSON.stringify({
                                type: 'redirect',
                                location: err.location
                            });

                            state.prerendering.dependencies.set(data_pathname, {
                                response: text(body),
                                body
                            });
                        }

                        return redirect_response(err.status, err.location);
                    }

This Redirect is imported in following way: import { Redirect } from '../../control.js';

which points to following file => my-app/nodemodules/.pnpm/@sveltejs+kit@2.5.18@sveltejs+vite-plugin-svelte@3.1.1_svelte@4.2.18_vite@5.3.3__svelte@4.2.18_vite@5.3.3/node_modules/@sveltejs/kit/src/runtime/control.js

So the fucntion which throws the Redirect and the function which deals with the instanceof check uses the same import and the same file in my opinion.

my-app/nodemodules/.pnpm/@sveltejs+kit@2.5.18@sveltejs+vite-plugin-svelte@3.1.1_svelte@4.2.18_vite@5.3.3__svelte@4.2.18_vite@5.3.3/node_modules/@sveltejs/kit/src/runtime/control.js

So I would guess if the import has the same filePath there must not be a dublication of the Redirect Instances. Or did I miss something?

Folder Structure node_modules

hi-ogawa commented 1 month ago

Thanks for cleaning up the repro and explanation.

This appears to me that it's still expected behavior and Sveletekit is not able to support preserveSymlinks: true.

From what I can see by running DEBUG=vite:resolve, your source code's import { redirect } from '@sveltejs/kit' is resolved to the symlink itself due to preserveSymlinks: true. You would see a log without .pnpm:

2024-08-01T06:28:12.963Z vite:resolve 0.17ms @sveltejs/kit -> /home/hiroshi/code/tmp/svelte-kit-issue--12139-II/node_modules/@sveltejs/kit/src/exports/index.js
2024-08-01T06:28:12.963Z vite:resolve 0.07ms /node_modules/@sveltejs/kit/src/exports/index.js -> /home/hiroshi/code/tmp/svelte-kit-issue--12139-II/node_modules/@sveltejs/kit/src/exports/index.js
2024-08-01T06:28:12.964Z vite:resolve 0.27ms ../runtime/control.js -> /home/hiroshi/code/tmp/svelte-kit-issue--12139-II/node_modules/@sveltejs/kit/src/runtime/control.js

here node_modules/@sveltejs/kit/src/runtime/control.js is defining export class Redirect { ... }, so that's the instance your source code is throwing.

On the other hand, somewhere in a different line, you would also see the one from .pnpm symlink:

2024-08-01T06:28:12.797Z vite:resolve 0.96ms ../control.js -> /home/hiroshi/code/tmp/svelte-kit-issue--12139-II/node_modules/.pnpm/@sveltejs+kit@2.5.18_@sveltejs+vite-plugin-svelte@3.1.1_svelte@4.2.18_vite@5.3.3__svelte@4.2.18_vite@5.3.3/node_modules/@sveltejs/kit/src/runtime/control.js

So, it's likely that Sveltekit somehow eneded up resolving/importing from "realpath" internally, so that'll be a distinct class Redirect.

Having Sveltekit side issue seems appropriate to me, so let me close this issue.