add types for stores

[TheComputerM]

Fixes #1556

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with npm test and lint the project with npm run lint

[TheComputerM]

Also imported PageContext from @sapper/app/types as to not re-declare it.

[ehrencrona]

LGTM :)

[bjon]

import { PageContext } from "@sapper/app/types"; doesn't seem to work with vscode. It still shows:

(alias) stores(): {
    preloading: Readable<boolean>;
    page: Readable<any>;
    session: Writable<any>;
}
import stores

[bjon]

server.ts is created like this:

polka() // You can also use Express
	.use(
		compression({ threshold: 0 }),
		sirv('static', { dev }),
		sapper.middleware()
	)
	.listen(PORT, err => {
		if (err) console.log('error', err);
	});

so opts in middleware, should be optional.

export function middleware(opts?: MiddlewareOptions): Handler;

[TheComputerM]

import { PageContext } from "@sapper/app/types"; doesn't seem to work with vscode. It still shows:

(alias) stores(): {
    preloading: Readable<boolean>;
    page: Readable<any>;
    session: Writable<any>;
}
import stores

I think PageContext has to be a type instead of an interface for this to work, please correct me if I am wrong.

[bjon]

@TheComputerM Both type or interface should work. I just don't understand the "magic" behind @sapper/app/types.
How can vscode access it?

[TheComputerM]

So, shall I convert @sapper/app/types to ./src/app/types?

[bjon]

That file doesn't exist either?

The only way I can get it to work, is to declare the namespace, directly in the index.d.ts

declare module '@sapper/app/types' {
  export interface PageContext {
    host: string;
    path: string;
    params: Record<string, string>;
    query: Record<string, string | string[]>;
    error?: Error;
  }
}

But now we're back to square one again :)

[TheComputerM]

This is the file

[bjon]

Yes, but only in the source. I can't find it anywhere in node_modules/sapper or src/node_modules/@sapper.

If its not included in the sapper dist, or auto-generated [@]sapper, I dont see how we can import it.

[TheComputerM]

So how do we fix this?

[bjon]

If @sapper/app/types and @sapper/internal/manifest-server are supposed to work, it might be a bug when sapper generates the code?

[bjon]

One workaround would be to add

declare module '@sapper/app/types' {
  export interface PageContext {
    host: string;
    path: string;
    params: Record<string, string>;
    query: Record<string, string | string[]>;
    error?: Error;
  }
}

and remove it, when the import works?

[bjon]

But im no expert here. I just want auto-complete to work :)

[TheComputerM]

Ok, If someone approves this, I will add this.

[bjon]

And also, export function start(opts: { target: Node }): Promise<void>; should be export function start(opts: { target: Element }): Promise<void>;, cause document.querySelector returns an Element

sapper.start({
  target: document.querySelector(`#sapper`),
});

[bjon]

To avoid compile error its should probably be export function start(opts: { target: Element | null }): Promise<void>;.
document.querySelector returns Element | null

or maybe better, change sapper-template to convert TS to

sapper.start({
  target: document.querySelector(`#sapper`) as Element,
});

[AnandChowdhary]

To avoid compile error its should probably be export function start(opts: { target: Element | null }): Promise<void>;.
document.querySelector returns Element | null

or maybe better, change sapper-template to convert TS to

sapper.start({
  target: document.querySelector(`#sapper`) as Element,
});

This is what we do, maybe it's better to ensure it exists instead of typecasting?

const target = document.querySelector("#sapper");

if (target)
  sapper.start({
    target,
    hydratable: true,
  });

[bjon]

Found one more. In @sapper/app goto(opts), should be optional

[ehrencrona]

This is a useful discussion; the handling of types right now seems wrong. I looked into how the types work: in the tsconfig.json @sapper is defined as an alias for the directory runtime/src:

"paths": {
	"@sapper/*": ["runtime/src/*"]
}

This is the reason why @sapper/internal references work in Sapper code, but not in project code. In project code there simply is no such thing; the only thing resolved under @sapper is what we put in the index.d.ts.

Therefore, those references from index.d.ts to @sapper/internal must be removed. I created #1604 for this. Once that is merged, it should be possible to apply this PR on top of #1604 (though I'm afraid #1604 might cause some controversy and could take time to merge).

[ehrencrona]

On the discussion around whether target in start is Node or an Element: An Element is a Node; Node is the most abstract class. start can handle Nodes so it should be target: Node.

Furthermore, I don't think start can handle target being null. If you can't find the DOM node you want to start the app in, that's an error, which is essentially what the type mismatch says.

[bencates]

Might be nice to parameterize these types to let users type the session and preloaded props.

[bencates]

	export interface Preload<Props = object, Session = any> {
		(this: PreloadContext, page: PageContext, session: Session): Props | Promise<Props>;
	}

[benmccann]

@TheComputerM this PR will need to be rebased

[bencates]

Sorry, this might be my fault. GitHub would only allow me to format half of my proposed changes as suggestions.

[bencates]

I don't think this works correctly unless we also parameterize MiddlewareOptions