solidjs / solid-start

SolidStart, the Solid app framework
https://start.solidjs.com
MIT License
4.93k stars 371 forks source link

[Bug?]: dup signIn import from @solid-mediaket/auth -> build error #1515

Closed billmartschenko closed 1 month ago

billmartschenko commented 1 month ago

Duplicates

Latest version

Current behavior 😯

Doing a signIn on the top level index page for a site.

Works fine if I import this way.

import { Session } from '@solid-mediakit/auth';
import { createSession, signIn } from '@solid-mediakit/auth/client';

Fails if I import this way.

import { Session, signIn } from '@solid-mediakit/auth';
import { createSession } from '@solid-mediakit/auth/client';

I didn't see documentation describing 2 ways to signIn. The change in import was inadvertent and happened via automatic import recommendation in vscode. Maybe doc issue or misread on my part. Seems subtle.

The failure is the attempted use of a server side node module in the client when I do pnpm build which is the same as vinxi build.

 ERROR  node_modules/.pnpm/vinxi@0.3.11_@opentelemetry+api@1.8.0_@types+node@20.12.12_ioredis@5.4.1_terser@5.31.0/node_modules/vinxi/runtime/http.js (70:9): "AsyncLocalStorage" is not exported by "__vite-browser-external", imported by "node_modules/.pnpm/vinxi@0.3.11_@opentelemetry+api@1.8.0_@types+node@20.12.12_ioredis@5.4.1_terser@5.31.0/node_modules/vinxi/runtime/http.js".

The import definitions are different. index.d.ts (fails) /Users/billm/dev/neutron/hoffa/frontend/dash/webapp/sstart/node_modules/.pnpm/@solid-mediakit+auth@2.0.11_@auth+core@0.29.0_@solidjs+meta@0.29.4_solid-js@1.8.17__@solidjs+_po46vxxkmqdaywwmto3pszjmbi/node_modules/@solid-mediakit/auth/index.d.ts

export declare function signIn(provider: SignInParams[0], options: SignInParams[1], authorizationParams: SignInParams[2], config: SolidAuthConfig, event: APIEvent): Promise<any>;

client.d.ts (works fine) /Users/billm/dev/neutron/hoffa/frontend/dash/webapp/sstart/node_modules/.pnpm/@solid-mediakit+auth@2.0.11_@auth+core@0.29.0_@solidjs+meta@0.29.4_solid-js@1.8.17__@solidjs+_po46vxxkmqdaywwmto3pszjmbi/node_modules/@solid-mediakit/auth/client.d.ts

export declare function signIn<P extends RedirectableProviderType | undefined = undefined>(providerId?: LiteralUnion<P extends RedirectableProviderType ? P | BuiltInProviderType : BuiltInProviderType>, options?: SignInOptions, authorizationParams?: SignInAuthorizationParams): Promise<Response>;

Expected behavior 🤔

I expected a single way to import the signIn function or better doc to show 2 ways to do it. Intuitively, makes sense to have 2 ways for client side and server side.

For me, I didn't read as an import from @solid-mediakit/auth and @solid-mediakit/auth/client to capture the distinction between server-side auth and client-side auth.

Steps to reproduce 🕹

Steps:

  1. pnpm create solid. (project name: test1; is solid start: Yes; template: with-authjs; typescript: Yes)
  2. cd test1. Edit src/routes/index.tsx to import signIn from "@solid-mediakit/auth" instead of the provided "@solid-mediakit/auth/client".
  3. pnpm i && pnpm build

Context 🔦

I was doing code refactoring after getting the with-authjs template working. I wasn't even working with auth at the time. I had introduced some other components and needed to refactor. As I imported signIn in a .tsx, the recommendation from vscode was the wrong one. I didn't even notice and had to bisect current code vs the original template to isolate.

Your environment 🌎

**System:**
- OS: macOS 13.6.7 (22G720)
- CPU: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz

**Binaries:**
- Node: 20.12.2
- pnpm: 9.1.2

**npmPackages:**

dependencies:
@auth/core 0.29.0
@babel/core 7.24.4
@solid-mediakit/auth 2.0.11
@solidjs/meta 0.29.4
@solidjs/router 0.13.3
@solidjs/start 1.0.0
solid-js 1.8.17
vinxi 0.3.11

devDependencies:
@types/node 20.12.12
esbuild 0.20.2
next-auth 4.24.7
postcss 8.4.38
typescript 5.4.5
vite 5.2.11
LukasGerm commented 1 month ago

I mean, is this now a bug in @solid-mediakit or just unclear behaviour? Because as far as I can understand now it works as expected but you were just not aware that two exports exist?

davedbase commented 1 month ago

Hi there, pinging @OrJDev to support on this. Was a similar issue opened in MediaKit?

OrJDev commented 1 month ago

Basically when importing from "/" - @solid-mediakit/auth is a server file, functions imported from here are server functions we use under the hood, you shouldn't import such functions from there.

the signIn function on /client dir is the actual function you should be using, it fully supports ssr, just use it - @solid-mediakit/auth/client

Related issue

https://github.com/solidjs-community/mediakit/issues/60

This has nothing to do with the solid start repo, its just me creating 2 functions with the same name and exporting both /:

billmartschenko commented 1 month ago

Yep, @OrJDev , I agree. The / equals internal and /client for both server and client functionality was not my expectation. It still wouldn't be, ha ha. Is there not a way to prevent the internal stuff from being visible? This all turned into a snakebite when I was just typing import statements and the autocompletion in vscode pulled from /.

Yes, @LukasGerm, I was not aware. More specifically, per @OrJDev, I was not aware to use the 2nd but not the 1st. That's what helped me not discover quickly what had happened.

I appreciate the feedback. I'm not pressing for a bug. This cost me a bit of time and my understanding was not clear even though I found a solution. I'm not a fan--just my opinion--that / is internal and /client is for both client and server functionality.

ryansolid commented 1 month ago

Ok since this is not related to SolidStart directly I will close. Follow up in that repo.