Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
7 tasks done
msprada opened this issue Jul 8, 2024 · 4 comments

Comments

@msprada
Copy link

msprada commented Jul 8, 2024

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 #17631 (comment).

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

  • install pnpm globally
  • install all dependencies by pnpm install
  • run dev mode 'pnpm run dev'
    - Click the "Throw Redirect" Button
    - An alert will be shown

Be aware

  • currently the instanceof check will fail because the preserveSymlinks is set to true as default
  • the instanceof check will succeed if preserveSymlinks is set to false

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
Copy link
Author

msprada commented Jul 8, 2024

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
Copy link
Collaborator

hi-ogawa commented Jul 9, 2024

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
Copy link
Author

msprada commented Jul 9, 2024

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/node_modules/.pnpm/@sveltejs+kit@2.5.18_@sveltejs[email protected][email protected][email protected][email protected][email protected]/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/node_modules/.pnpm/@sveltejs+kit@2.5.18_@sveltejs[email protected][email protected][email protected][email protected][email protected]/node_modules/@sveltejs/kit/src/runtime/control"

The instance check which fails is located in following file:
my-app/node_modules/.pnpm/@sveltejs[email protected]@sveltejs[email protected][email protected][email protected][email protected][email protected]/node_modules/@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/node_modules/.pnpm/@sveltejs+kit@2.5.18_@sveltejs[email protected][email protected][email protected][email protected][email protected]/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/node_modules/.pnpm/@sveltejs+kit@2.5.18_@sveltejs[email protected][email protected][email protected][email protected][email protected]/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
Copy link
Collaborator

hi-ogawa commented Aug 1, 2024

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/@[email protected]_@[email protected][email protected][email protected][email protected][email protected]/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.

@hi-ogawa hi-ogawa closed this as completed Aug 1, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants