Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

add types for stores #1568

Closed
wants to merge 5 commits into from
Closed

add types for stores #1568

wants to merge 5 commits into from

Conversation

TheComputerM
Copy link
Contributor

@TheComputerM TheComputerM commented Sep 27, 2020

Solves #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

@benmccann
Copy link
Member

Please change the formatting back to match the original (e.g. tabs instead of spaces)

@TheComputerM
Copy link
Contributor Author

Shall I also add a .editorconfig to enforce formatting in the editor?

@TheComputerM
Copy link
Contributor Author

Please inform me if you want me to add more type definitions.


interface Stores {
preloading: {
subscribe(preloading: boolean): () => void;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are ready-made types for Stores in svelte that you could reuse that give a more complete type definition for subscribe (there's an optional second parameter and a return value that are missing here):

I think if you just do

import { Readable, Writable } from 'svelte/store';

...
	preloading: Readable<boolean>;

you should get the correct definitions for subscribe etc for free.

For the page store, there is also already a PageContext interface, so you could declare it as

	import { PageContext } from '@sapper/app/types';
...
	page: Writeable<PageContext>;

Copy link
Contributor Author

@TheComputerM TheComputerM Sep 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, Thanks.

@benmccann
Copy link
Member

@TheComputerM this PR will need to be rebased

@TheComputerM
Copy link
Contributor Author

Sorry, I had forgotten this.

import { Readable, Writable } from "svelte/store";
import { PageContext } from "@sapper/app/types";

declare module "@sapper/app";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there were just removed in #1598. you might need to rebase against master

href: string,
opts: { noscroll?: boolean; replaceState?: boolean }
): Promise<void>;
export function prefetch(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you leave the method formatting the way it was?

@benmccann
Copy link
Member

The linting and tests are failing with this PR

@TheComputerM TheComputerM deleted the thecomputerm/types branch October 13, 2020 03:46
@TheComputerM
Copy link
Contributor Author

I am going to open a new PR because this is getting cluttered.

@TheComputerM
Copy link
Contributor Author

New PR: #1601

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

Successfully merging this pull request may close these issues.

3 participants