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

Module '"@sveltejs/kit"' declares 'RequestEvent' locally, but it is not exported #4805

Closed
nstuyvesant opened this issue May 3, 2022 · 18 comments · Fixed by #4809
Closed
Labels
help wanted PRs welcomed. The implementation details are unlikely to cause debate types / typescript
Milestone

Comments

@nstuyvesant
Copy link
Contributor

Describe the bug

Getting a TypeScript warning Module '"@sveltejs/kit"' declares 'RequestEvent' locally, but it is not exported for this line of code...

import type { RequestEvent } from '@sveltejs/kit'

Why do I need it? I have two functions in my hooks.ts called from handle() that pass the event as a parameter...

// Attach authorization to each server request (role may have changed)
async function attachUserToRequest(sessionId: string, event: RequestEvent) {
  const sql = `SELECT * FROM get_session($1);`
  const { rows } = await query(sql, [sessionId])
  if (rows?.length > 0) {
    event.locals.user = rows[0].get_session
  }
}

function deleteCookieIfNoUser(event: RequestEvent, response: Response) {
  if (!event.locals.user) {
    response.headers['Set-Cookie'] = `session=; Path=/; HttpOnly; SameSite=Lax; Expires=${new Date().toUTCString()}`
  }
}

export const handle: Handle = async ({ event, resolve }) => {

  const cookies = cookie.parse(event.request.headers.get('Cookie') || '')
  if (cookies.session) {
    await attachUserToRequest(cookies.session, event)
  }

  const response = await resolve(event)

  deleteCookieIfNoUser(event, response)
  return response
}

Not sure if there was a recent change but my code has been in place for some time.

Reproduction

See code above...

Logs

No response

System Info

System:
    OS: macOS 12.3.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 1.53 GB / 32.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.15.0 - /opt/homebrew/bin/node
    Yarn: 1.22.18 - /opt/homebrew/bin/yarn
    npm: 8.8.0 - /opt/homebrew/bin/npm
  Browsers:
    Chrome: 101.0.4951.54
    Firefox: 99.0.1
    Safari: 15.4
  npmPackages:
    @sveltejs/adapter-node: latest => 1.0.0-next.73 
    @sveltejs/kit: latest => 1.0.0-next.324 
    svelte: ^3.48.0 => 3.48.0

Severity

annoyance

Additional Information

My code works but the warning is a little annoying. More importantly, I think there is value for the RequestEvent type. Would be great to have it exported like Handle and GetSession.

@elliott-with-the-longest-name-on-github
Copy link
Contributor

Easy change. Happy to open a PR if a maintainer is okay with this being an exported type. (I would think it should be, since it's part of a public API.)

@nstuyvesant
Copy link
Contributor Author

Thanks!

@benmccann
Copy link
Member

Seems reasonable enough to me

There was a desire to only expose what was necessary and start out with stuff being private. Your use case seems to make sense to me though. And we exposed some other stuff in #4461

@benmccann benmccann added this to the 1.0 milestone May 3, 2022
@benmccann benmccann added the help wanted PRs welcomed. The implementation details are unlikely to cause debate label May 3, 2022
@elliott-with-the-longest-name-on-github
Copy link
Contributor

Ben, I'll take care of this and open a PR today.

More widely, it seems to me like the input and return types of the public API should all be exposed to the user. From a TypeScript perspective, it's always a frustrating user experience to have access to an exported API but not have access to that API's input and output types.

From a dependency perspective, I don't think it makes a difference either way -- whether you expose the type publicly or not, users are still depending on the implicit type, so why not make it easier on them? 🙂

Would you like me to audit the exported function types for functions with unexported input/output types and create an issue for discussion if there are any remaining?

@elliott-with-the-longest-name-on-github
Copy link
Contributor

Should be closed by #4809.

@carusog
Copy link
Contributor

carusog commented May 4, 2022

I'd like to second @tcc-sejohnson here.

…the input and return types of the public API should all be exposed to the user. From a TypeScript perspective, it's always a frustrating user experience to have access to an exported API but not have access to that API's input and output types.

This is, word by word, what makes (to me at least) currently uneasy working with TS in Svelte Kit. It's OK that the framework itself is not a TypeScript first framework, but to those that would like to use it in a type safe env, these improvements would make our lives easier.
I am not at all a TS expert (learning and loving it), but please, let me know if I can help in any way. Happy to join the discussion and contribute to it as I can.

@delhanty
Copy link

delhanty commented May 15, 2022

Just a comment for reference:

I got this exact problem today from the get-go building the boilerplate project suggested by kit.svelte.dev

npm init svelte my-app
cd my-app
npm install

then

npm run check

Edit: tag self @delhanty for future issue "mentioned" search.

@benmccann
Copy link
Member

Can you be clearer about what the boilerplate project is? Did you run npm init svelte? What options did you choose?

@delhanty
Copy link

delhanty commented May 15, 2022

Did you run npm init svelte?

Yes

I chose "SvelteKit demo app" and then

  1. Typescript
  2. ESLint: Yes
  3. Prettier: Yes
  4. Playwright: Yes

If it makes a difference, I develop with pnpm. (With pnpm v7, pnpm init svelte appears broken though.)

cd my-app
pnpm install
pnpm build
pnpm check

pnpm build is necessary to create .svelte-kit/tsconfig.json, which is extended by tsconfig.json.

As an aside, there were also Prettier issues with pnpm check. I could list these in another comment if it were of interest.

@delhanty
Copy link

delhanty commented May 15, 2022

Oh, there are also other svelte check Typescript errors when checking the TODO route:

/Users/paul/dev/tmp/my-app/src/routes/todos/index.ts:4:45
Error: Binding element 'locals' implicitly has an 'any' type. 

export const get: RequestHandler = async ({ locals }) => {
	// locals.userid comes from src/hooks.js


/Users/paul/dev/tmp/my-app/src/routes/todos/index.ts:31:46
Error: Binding element 'request' implicitly has an 'any' type. 

export const post: RequestHandler = async ({ request, locals }) => {
	const form = await request.formData();

@delhanty

This comment was marked as off-topic.

@delhanty

This comment was marked as off-topic.

@delhanty

This comment was marked as off-topic.

@delhanty

This comment was marked as off-topic.

@delhanty

This comment was marked as off-topic.

@delhanty

This comment was marked as off-topic.

@delhanty

This comment was marked as off-topic.

@benmccann
Copy link
Member

Thanks for the bug reports. I filed them as new issues in #4937 and #4938. I collapsed the comments here in favor of those issues to keep this thread from getting off-topic

With pnpm v7, pnpm init svelte appears broken though

When you run it, it tells you to do pnpm create instead of pnpm init and then it works

I had sort of assumed that the boilerplate generator source codenpm init svelte my-app was in the package github.com/sveltejs/kit/tree/master/packages/create-svelte

It is. That's what's run when you do npm init svelte

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted PRs welcomed. The implementation details are unlikely to cause debate types / typescript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants