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

Add a shape to the TS config files #1466

Merged
merged 3 commits into from
Nov 2, 2020
Merged

Conversation

orta
Copy link
Contributor

@orta orta commented Oct 31, 2020

Changes

Adds a JSDoc comment to add types to the config files with the TypeScript CRA templates

After:
Screen Shot 2020-10-31 at 1 17 40 PM

Testing

Tested in my own CRA project, which I ported

Docs

Not needed

Question:

Do you want this on all the JS templates too? IMO it's good for everyone, but I'm obv biased ;)

@vercel
Copy link

vercel bot commented Oct 31, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/r7n10mnpt
✅ Preview: https://snowpack-git-typeconfig.pikapkg.vercel.app

@FredKSchott
Copy link
Owner

Love this! +1 for adding to the JS templates as well.

1 comment: SnowpackUserConfig is the proper typing for a user-provided config values, which is a deep partial of the "cleaned" SnowpackConfig type which has defaults applied.

@orta
Copy link
Contributor Author

orta commented Nov 2, 2020

Cool, great point - updated to that type, and added to al the JS templates too

@FredKSchott
Copy link
Owner

Thanks! If you run yarn test -u in the root project directory, it will update snapshots and your tests should pass

@orta
Copy link
Contributor Author

orta commented Nov 2, 2020

thanks!

@ljani
Copy link
Contributor

ljani commented Nov 10, 2020

SnowpackUserConfig is the proper typing for a user-provided config values

Is it, or have I misunderstood something? Please try the following example:

import type {SnowpackUserConfig} from "snowpack";

const config: SnowpackUserConfig = {
  mount: {
    "public": "/",
    "src": "/_dist_",
  },
  plugins: [
    "@snowpack/plugin-react-refresh",
    "@snowpack/plugin-dotenv",
    "@snowpack/plugin-typescript",
  ],
};

export default config;

I get a lot of errors:

snowpack-example.ts:6:5 - error TS2559: Type 'string' has no properties in common with type 'DeepPartial<MountEntry>'.

6     "src": "/_dist_",
      ~~~~~

  ../../node_modules/snowpack/lib/types/snowpack.d.ts:6:38
    6 export declare type DeepPartial<T> = {
                                           ~
    7     [P in keyof T]?: T[P] extends Array<infer U> ? Array<DeepPartial<U>> : T[P] extends ReadonlyArray<infer U> ? ReadonlyArray<DeepPartial<U>> : DeepPartial<T[P]>;
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    8 };
      ~
    The expected type comes from property 'src' which is declared here on type 'DeepPartial<Record<string, MountEntry>>'

snowpack-example.ts:9:5 - error TS2559: Type 'string' has no properties in common with type 'DeepPartial<SnowpackPlugin>'.

9     "@snowpack/plugin-react-refresh",
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

snowpack-example.ts:10:5 - error TS2559: Type 'string' has no properties in common with type 'DeepPartial<SnowpackPlugin>'.

10     "@snowpack/plugin-dotenv",
       ~~~~~~~~~~~~~~~~~~~~~~~~~

snowpack-example.ts:11:5 - error TS2559: Type 'string' has no properties in common with type 'DeepPartial<SnowpackPlugin>'.

11     "@snowpack/plugin-typescript",
       ~~~~~~~~~~~~

EDIT: Otherwise, supporting TypeScript in configuration is wonderful!

@FredKSchott
Copy link
Owner

Ah good catch, now that this is user-facing DeepPartial<SnowpackConfig> doesn't really cut it anymore. Improved these types in this PR: #1577

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

Successfully merging this pull request may close these issues.

3 participants