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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug?]: The example in the docs doesn't seam to work. #1466

Open
2 tasks done
RonenEizen opened this issue May 2, 2024 · 14 comments
Open
2 tasks done

[Bug?]: The example in the docs doesn't seam to work. #1466

RonenEizen opened this issue May 2, 2024 · 14 comments
Labels
bug Something isn't working

Comments

@RonenEizen
Copy link

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 馃槸

The example in the documentation doesn't seam to work.

declare module "@solidjs/start/server" {
  interface RequestEventLocals {
    myNumber: number;
    someString: string;
  }
}

locals property within the RequestEvent doesn't have any types declared in global.d.ts. For example in one of the routes:

import { getRequestEvent } from 'solid-js/web'

const getMessage = cache(async () => {
  'use server'
  const event = getRequestEvent()
  // event?.locals is not typed
  return
}, 'message')

export const route = {
  load: () => getMessage(),
}

and my global.d.ts has

declare module '@solidjs/start/server' {
  interface RequestEventLocals {
    myNumber: number
    someString: string
  }
}

Expected behavior 馃

No response

Steps to reproduce 馃暪

Steps:

  1. Create an solid-start app using one of the templates
  2. Declare '@solidjs/start/server' within the global.d.ts file
  3. Try accessing event.locals.<SOMETHING> within a route function

Context 馃敠

No response

Your environment 馃寧

No response

@RonenEizen RonenEizen added the bug Something isn't working label May 2, 2024
@RonenEizen RonenEizen changed the title [Bug?]: [Bug?]: The example in the documentation doesn't seam to work. May 2, 2024
@RonenEizen RonenEizen changed the title [Bug?]: The example in the documentation doesn't seam to work. [Bug?]: The example in the docs doesn't seam to work. May 2, 2024
@ryansolid
Copy link
Member

I am going to ask for a reproduction. We have an example template experiments that has this usage in it and it appears to be working.

https://github.com/solidjs/solid-start/blob/main/examples/experiments/src/entry-server.tsx#L4-L9
https://github.com/solidjs/solid-start/blob/main/examples/experiments/src/routes/index.tsx#L12-L13

(the example uses an undeclared value, but switch it to n/s shows the right types).

@RonenEizen
Copy link
Author

RonenEizen commented May 2, 2024

@ryansolid It does work if declared within entry-server.tsx. I created a repository based on a basic template. The types still don't work when declared within global.d.ts file. Look at the line 9 in the routes/index.tsx file.

@AirBorne04
Copy link
Contributor

@RonenEizen for me it started to work also in the global.d.ts when changing the
/// <reference types="@solidjs/start/env" />
to
import "@solidjs/start/env";
(make sure to restart ts server to see the effect)

I am not sure why this is the case though.

@ryansolid
Copy link
Member

Oh.. hmm so this is a weird typescript-ism.. Lovely. Not sure what to do with this then.

@AirBorne04
Copy link
Contributor

I think I got to the bottom of the behaviour, it is described here ambient vs augmented modules. Just adding an export {} is fixing the issue but seems weird to just have it dangling there with no meaning.
Checking the Astro repo they are using declare namespace App for typing their locals -> docs, i am not sure there is an easier way around this.

@RonenEizen
Copy link
Author

I am personally fine with the workaround. Should I keep this issue? Or would you rather have it resolved?

@AirBorne04
Copy link
Contributor

AirBorne04 commented May 5, 2024

@ryansolid i guess it would make sense to rework to namespace? i threw together the needed changes here #1469 -> than the new way to overwrite the RequestEventLocals would look like this:

declare namespace App {
  interface RequestEventLocals {
    myNumber: number;
    someString: string;
  }
}

The old first line reference is not needed.

@rmacfie
Copy link

rmacfie commented May 12, 2024

I think the only thing needed to get the @solidjs/start/env types working is to include ./env.d.ts in the exports property of package.json.

The schema documentation for package.json says:

The "exports" field is used to restrict external access to non-exported module files, also enables a module to import itself using "name".

Edit: For TypeScript <5 you may also need to add it to the typesVersions property of package.json.

Edit again: To clarify, I'm talking about the package.json of solid-start.

@AirBorne04
Copy link
Contributor

AirBorne04 commented May 12, 2024

@rmacfie I did a couple of tests to figure it out and I did not test the specific suggestion. Did you test this?

From my understanding the problem has nothing to do with the actual reference vs. import. Just TypeScript switching the module interpretation from ambient (with reference) to augmented with any import / export statement, no need to import the env.d.ts file at all.

@rmacfie
Copy link

rmacfie commented May 12, 2024

@rmacfie I did a couple of tests to figure it out and I did not test the specific suggestion. Did you test this?

Yes, I modified node_modules/@solidjs/start/package.json to include ./env.d.ts in the exports section. This makes references to @solidjs/start/env work.

If I understand correctly, once exports in package.json is declared it will act as a white-list for Typescript, so it's logical that @solidjs/start/env just doesn't work correctly until this is fixed.

@AirBorne04
Copy link
Contributor

@rmacfie can you share the test setup, i tried making the stated modifications but it does not work for me.

@rmacfie
Copy link

rmacfie commented May 13, 2024

@rmacfie can you share the test setup, i tried making the stated modifications but it does not work for me.

I made a proof of concept of changes in solid-start and the basic example with some dummy extensions to the ImportMetaEnv and RequestEventLocals types. You can see the diff here:
main...rmacfie:solid-start:issue-1466-poc

In examples/basic/src/routes/about.tsx you can see

  console.log(import.meta.env.FOO);
  console.log(requestEvent.locals.foo);

The foo properties are typed according to the extensions declared in examples/basic/src/global.d.ts.

@rmacfie
Copy link

rmacfie commented May 13, 2024

I think I got to the bottom of the behaviour, it is described here ambient vs augmented modules. Just adding an export {} is fixing the issue but seems weird to just have it dangling there with no meaning.

It may look weird but I believe this is the recommended pattern to follow for this situation.

It has probably worked at some point without export {}, but a change in tsconfig.json broke it, possible the addition of "isolatedModules": true.

@AirBorne04
Copy link
Contributor

@rmacfie

@rmacfie can you share the test setup, i tried making the stated modifications but it does not work for me.

I made a proof of concept of changes in solid-start and the basic example with some dummy extensions to the ImportMetaEnv and RequestEventLocals types. You can see the diff here: main...rmacfie:solid-start:issue-1466-poc

In examples/basic/src/routes/about.tsx you can see

  console.log(import.meta.env.FOO);
  console.log(requestEvent.locals.foo);

The foo properties are typed according to the extensions declared in examples/basic/src/global.d.ts.

Yes that is working because adding an empty export {}; in examples/basic/src/global.d.ts is telling typescript to interpret as an module augmentation. No other change needed from my testing experience.

I think I got to the bottom of the behaviour, it is described here ambient vs augmented modules. Just adding an export {} is fixing the issue but seems weird to just have it dangling there with no meaning.

It may look weird but I believe this is the recommended pattern to follow for this situation.

It has probably worked at some point without export {}, but a change in tsconfig.json broke it, possible the addition of "isolatedModules": true.

The typescript docs state This module declaration syntax becomes a module augmentation when the file is a module, meaning it has a top-level import or export statement (or is affected by [--moduleDetection force or auto](https://www.typescriptlang.org/tsconfig#moduleDetection)): and do not indicate any involvement of the "isolatedModules": true field.

Honestly I have not seen/found a better option to fix this inconvenience than how i addressed it with the PR #1469

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants