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

generated tsconfig.json #4118

Merged
merged 11 commits into from
Feb 28, 2022
Merged

generated tsconfig.json #4118

merged 11 commits into from
Feb 28, 2022

Conversation

Rich-Harris
Copy link
Member

Groundwork for #647. This adds a generated .svelte-kit/tsconfig.json and checks that the user's config (if such exists) extends it.

This allows us to make changes like #3064 that existing apps can take advantage of (rather than only newly created apps), including the change I have planned that will address #647.

It also allows us to base certain values on the user's configuration. For example, if someone were to specify a different lib folder...

// svelte.config.js
export default {
  kit: {
    files: {
      lib: 'src/my-stuff'
    }
  }
};

...then we can automatically generate the correct alias:

{
  "paths": {
    "$lib": ["../src/my-stuff"],
    "$lib/*": ["../src/my-stuff/*"]
  }
}

I'm not sure how much of the existing compilerOptions is necessary, but I figured I'd keep it there for now unless people have thoughts.

Please don't delete this checklist! 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 pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Feb 24, 2022

🦋 Changeset detected

Latest commit: dd83df1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
create-svelte Patch
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

module: 'es2020',
lib: ['es2020', 'DOM'],
target: 'es2020',
// svelte-preprocess cannot figure out whether you have a value or a type, so tell TypeScript
Copy link
Member

Choose a reason for hiding this comment

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

Are we worried about losing these comments in the generated tsconfig file that gets included? Does tooling generally provide a way to look at the file being extended, and do we care if that becomes less readable.

Or, another way of asking it: was the only reason for the user-visible comments so that people could know which config options not to mess with?

Also sort of relatedly: Do you foresee us potentially wanting to warn or error out if the user config does override certain essential values from the generated config?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not particularly worried about the comments. Though I do think it's probably a good idea to warn if the user specifies incompatible values (as already happens with compilerOptions.paths)

@Conduitry
Copy link
Member

Is there anything similar we'd want to do with jsconfig.json? Does that support extends in the same way?

@Rich-Harris
Copy link
Member Author

jsconfig.json works exactly the same way, yeah (in fact i've been testing all this with a jsconfig)

@Conduitry
Copy link
Member

Conduitry commented Feb 25, 2022

Oh whoops, I missed in the diff that you'd updated the jsconfig in the starter templates as well to extend the same file. Never mind, nice!

@benmccann
Copy link
Member

I left a few stylistic comments, but LGTM

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@janosh
Copy link
Contributor

janosh commented Mar 4, 2022

I'm getting warnings in lots of projects following this PR.

Your tsconfig.json should extend the configuration generated by SvelteKit:
{
  "extends": "./.svelte-kit/tsconfig.json"
}

Just adding the above lines to tsconfig.json doesn't seem like a good solution as it would break all CI scripts that run tests without calling svelte-kit dev/build first:

Error: failed to resolve "extends":"./.svelte-kit/tsconfig.json"

What's the recommended setup now?

@Conduitry
Copy link
Member

I'd say the recommendation is to wait for #4182 to be released.

@baseballyama baseballyama mentioned this pull request Mar 6, 2022
5 tasks
@ghost
Copy link

ghost commented Mar 6, 2022

After this PR, I am now unable to resolve imports to asset files. It seems that the generated tsconfig defaults to only including *.js, *.ts, and *.svelte files. Unfortunately this also means that importing such files as assets/styles.css result in a vite error when building the project.

I have resolved this issue by adding the following to my tsconfig:

{
    "extends": "./.svelte-kit/tsconfig.json",
    "include": ["src/**/*"],
    "exclude": [],
}

I do think that this makes more sense than only allowing .js, .ts, and .svelte files, because many users are going to want to make use of vite's asset handling features.

I think it would be beneficial and avoid some confusion to implement the following behaviour as default:

  • include all files inside the directory specified in config.kit.files.assets
  • include all .js, .ts, and .svelte files inside src/ (existing behaviour) and inside the hooks, lib, and routes folders specified in config.kit.files (in case the user specifies custom locations for these folders)

Edit

It seems this error occurs specifically when using baseUrl setting in tsconfig.json (where the vite-tsconfig-paths package is required to resolve baseurl imports).

However, the existing behaviour of the tsconfig generation still does not solve the problem of custom paths as specified inside config.kit.files. For example, if I set routes to be a folder routes/ in the root directory of my project, this directory is not included inside the tsconfig project.

@benmccann
Copy link
Member

@applemonkey496 could you file a new issue about this so that we can track it? It will get lost as a comment on a closed PR

EthanThatOneKid added a commit to EthanThatOneKid/acmcsuf.com that referenced this pull request Mar 13, 2022
Recommended to wait on this <sveltejs/kit#4118 (comment)>
EthanThatOneKid added a commit to EthanThatOneKid/acmcsuf.com that referenced this pull request Mar 16, 2022
Recommended to wait on this <sveltejs/kit#4118 (comment)>
EthanThatOneKid added a commit to EthanThatOneKid/acmcsuf.com that referenced this pull request Mar 17, 2022
* Added Vitest testing to about page

* Added tests for basic rendering on each major acmcsuf.com page

* Ran `npm run lint`

* Fixed all lint errors that came along with upgrading the sveltekit deps

* Added first group of tests for `/lib/ical/common`

* Update CONTRIBUTING.md

* Shorted npm run test to npm t in npm run all

* Removed unnecessary environment comment

* Added PR number to cache name

* Will this fix the lint errors?

* Update tsconfig.json

Recommended to wait on this <sveltejs/kit#4118 (comment)>

* Added Vitest testing to about page

* Added tests for basic rendering on each major acmcsuf.com page

* Ran `npm run lint`

* Fixed all lint errors that came along with upgrading the sveltekit deps

* Added first group of tests for `/lib/ical/common`

* Update CONTRIBUTING.md

* Shorted npm run test to npm t in npm run all

* Removed unnecessary environment comment

* Added PR number to cache name

* Will this fix the lint errors?

* Update tsconfig.json

* Ran `npm run format`

* Delete unused dep

* Imported `LoadInput` and `LoadOutput` from '@sveltejs/kit/types/internal' (for now)

* Upgraded dependencies again real quick

* Path parameters are always passed as string

* Resolved #337 (comment)

* Resolved <#337 (comment)>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants