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

instanceof ActionFailure refers to the wrong object. #10361

Closed
Curstantine opened this issue Jul 10, 2023 · 8 comments
Closed

instanceof ActionFailure refers to the wrong object. #10361

Curstantine opened this issue Jul 10, 2023 · 8 comments

Comments

@Curstantine
Copy link

Curstantine commented Jul 10, 2023

Describe the bug

When you use instanceof on an ActionFailure class, it fails with a TypeError: Right-hand side of 'instanceof' is not an object.

Reproduction

https://gitlab.com/Curstantine/sveltekit-instanceof-runtime

Logs

TypeError: Right-hand side of 'instanceof' is not an object
    at default (/home/<REDACTED>/Code/Experimental/sveltekit-instanceof-runtime/src/routes/+page.server.js:15:17)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Module.handle_action_json_request (/home/<REDACTED>/Code/Experimental/sveltekit-instanceof-runtime/node_modules/.pnpm/@sveltejs+kit@1.22.1_svelte@4.0.5_vite@4.4.2/node_modules/@sveltejs/kit/src/runtime/server/page/actions.js:57:16)
    at async resolve (/home/<REDACTED>/Code/Experimental/sveltekit-instanceof-runtime/node_modules/.pnpm/@sveltejs+kit@1.22.1_svelte@4.0.5_vite@4.4.2/node_modules/@sveltejs/kit/src/runtime/server/respond.js:412:18)
    at async Module.respond (/home/<REDACTED>/Code/Experimental/sveltekit-instanceof-runtime/node_modules/.pnpm/@sveltejs+kit@1.22.1_svelte@4.0.5_vite@4.4.2/node_modules/@sveltejs/kit/src/runtime/server/respond.js:279:20)
    at async file:///home/<REDACTED>/Code/Experimental/sveltekit-instanceof-runtime/node_modules/.pnpm/@sveltejs+kit@1.22.1_svelte@4.0.5_vite@4.4.2/node_modules/@sveltejs/kit/src/exports/vite/dev/index.js:505:22

System Info

System:
    OS: Linux 6.4 Arch Linux
    CPU: (4) x64 Intel(R) Core(TM) i5-3210M CPU @ 2.50GHz
    Memory: 1.95 GB / 7.65 GB
    Container: Yes
    Shell: 3.6.1 - /bin/fish
  Binaries:
    Node: 20.4.0 - /usr/bin/node
    Yarn: 1.22.19 - /usr/bin/yarn
    npm: 9.8.0 - /usr/bin/npm
    pnpm: 8.6.6 - /usr/bin/pnpm
  npmPackages:
    @sveltejs/adapter-auto: ^2.0.0 => 2.1.0 
    @sveltejs/kit: ^1.20.4 => 1.22.1 
    svelte: ^4.0.0 => 4.0.5 
    vite: ^4.3.6 => 4.4.2 

Severity

annoyance

Additional Information

This is most likely due to this "grotesque" hack not working.

This is apparent when you directly call an ActionFailure.
e.g.

import { ActionFailure } from "@sveltejs/kit";

const x = new ActionFailure(422, { message: "Deformed request" })`; // fails here
console.debug(x instanceof ActionFailure);
@david-plugge
Copy link
Contributor

The actual classes are not exported, just their types. This is by design and should error on the type level, no idea when this stopped doing so.

@Curstantine
Copy link
Author

Curstantine commented Jul 11, 2023

True, only the types are exported. But instanceof equality should work(?) according to the comment I mentioned before.

This instanceof equality would be nice to have for cases as I shown in the reproduction example. Or the export in @sveltejs/kit should mark the type as so. It's a bit confusing to the external consumers.

@david-plugge
Copy link
Contributor

That comment is about internal stuff. The classes are loaded in nodejs and vite which means they are not the exact same class/reference.

@Curstantine
Copy link
Author

Hmm, I see. I think it would be nice to export those classes only as types so the typescript server would complain while trying to instantiate them.

I'll leave the issue open for now.

@tomatrow
Copy link

This can be used as a temporary workaround.

import { fail, type ActionFailure as ActionFailureType } from "@sveltejs/kit"

interface Class<T> extends Function {
	new (...args: any[]): T
}

export const ActionFailure = fail(Infinity).constructor as unknown as Class<ActionFailureType>

@SBHattarj
Copy link

SBHattarj commented Oct 23, 2023

You can actually do something like this which I think looks better( I was trying to get the constructor for a library of mine )

import { fail, type ActionFailure as ActionFailureType } from "@sveltejs/kit"

export const ActionFailure = fail(Infinity).constructor as typeof ActionFailureType

But it'd be great if we didn't had to do this honestly.

@dummdidumm dummdidumm added this to the 2.0 milestone Dec 9, 2023
@dummdidumm
Copy link
Member

Adding this to the 2.0 milestone - it would be good to fix the type generation or the source of the type generation such that ActionFailure is no longer an interface, which is wrong

dummdidumm added a commit that referenced this issue Dec 11, 2023
- add ActionFailure interface and use that publicly instead of the class. Prevents the false impression that you could do "instanceof" on the return type, fixes #10361
- add uniqueSymbol to ActionFailure instance so we can distinguish it from a regular return with the same shape
- fix setup to actually run test/types
dummdidumm added a commit that referenced this issue Dec 11, 2023
* fix: Adjust fail method and ActionFailure type

- add ActionFailure interface and use that publicly instead of the class. Prevents the false impression that you could do "instanceof" on the return type, fixes #10361
- add uniqueSymbol to ActionFailure instance so we can distinguish it from a regular return with the same shape
- fix setup to actually run test/types

* test

* regenerate types

---------

Co-authored-by: Rich Harris <rich.harris@vercel.com>
@Rich-Harris
Copy link
Member

fixed in #11261, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants