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

fix: correctly configure module and noEmit tsconfig and jsconfig properties #11029

Merged
merged 9 commits into from
Nov 14, 2023

Conversation

eltigerchino
Copy link
Member

@eltigerchino eltigerchino commented Nov 13, 2023

fixes #11023
fixes #9552

This PR fixes the errors found in tsconfig.json when scaffolding a typescript library project then running pnpm check.

Cannot write file '/src/lib/index.js' because it would overwrite input file.

This one is fixed by providing a index.ts file instead of .js in the example lib folder. EDIT: and adding noEmit to tsconfig

Option 'module' must be set to 'NodeNext' when option 'moduleResolution' is set to 'NodeNext'.

This one is fixed by adding the module: NodeNext to the library project tsconfig.json and jsconfig.json files. EDIT: setting module: bundler

Thanks to @hyunbinseo

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 pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Copy link

changeset-bot bot commented Nov 13, 2023

🦋 Changeset detected

Latest commit: 391c8a4

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

This PR includes changesets to release 1 package
Name Type
create-svelte 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

@eltigerchino eltigerchino changed the title fix: add module NodeNext to tsconfig.json when scaffolding library projects fix: add typescript errors in tsconfig.json when scaffolding library projects Nov 13, 2023
@eltigerchino eltigerchino changed the title fix: add typescript errors in tsconfig.json when scaffolding library projects fix: add module config in tsconfig.json when scaffolding library projects Nov 13, 2023
@eltigerchino eltigerchino changed the title fix: add module config in tsconfig.json when scaffolding library projects fix: set module config to NodeNext in tsconfig.json when scaffolding library projects Nov 13, 2023
@dummdidumm
Copy link
Member

This one is fixed by providing a index.ts file instead of .js in the example lib folder.

This won't help though if you're creating a library using JSDoc, you won't have a .ts file then. This is related to #9552 and the fix is something like this: #9552 (comment)

@eltigerchino
Copy link
Member Author

This won't help though if you're creating a library using JSDoc, you won't have a .ts file then. This is related to #9552 and the fix is something like this: #9552 (comment)

Based on https://vitejs.dev/guide/features.html#transpile-only and https://www.typescriptlang.org/tsconfig#noEmit it would seem that noEmit is the right option here as per your comment in #9552 . I don't think it will affect Vite as they use esbuild instead of tsc to transpile to Javascript. This comment #9552 (comment) also suggests that other Vite based templates are using noEmit.

A bit off-topic: reading Vite's docs about tsc --noEmit makes me wonder if we should have type checking as part of the default npm build script before vite build.

@eltigerchino
Copy link
Member Author

I've added noEmit to the templates but wonder what's the difference between putting it there and putting it in the generated one in .svelte-kit

@eltigerchino eltigerchino changed the title fix: set module config to NodeNext in tsconfig.json when scaffolding library projects fix: set module: NodeNext and noEmit in tsconfig.json when scaffolding library projects Nov 13, 2023
@eltigerchino eltigerchino changed the title fix: set module: NodeNext and noEmit in tsconfig.json when scaffolding library projects fix: set module: NodeNext and noEmit in tsconfig.json when scaffolding projects Nov 13, 2023
@dummdidumm
Copy link
Member

The generated one means that people updating SvelteKit get the fix, while putting it in create-svelte means people only get it when starting a new project (or they manually update their tsconfig).
I think in this instance it makes sense to have it in the generated tsconfig. I can't imagine a case where someone would use that same tsconfig to start a completely different build where the setting actually makes a difference. If Vite ignores it in all instances that matter, then it's probably fine. If we're paranoid/unsure if there couldn't be a case for not wanting it, then it should go into create-svelte like you did now.

@benmccann
Copy link
Member

benmccann commented Nov 13, 2023

Setting noEmit in the generated file makes sense to me assuming we can do it just for JS projects.

I'm not sure about "moduleResolution": "NodeNext" though. Won't that start requiring users to have a .js extension? In SvelteKit 2, I think we'd like to set it to bundler and require TypeScript 5

@eltigerchino
Copy link
Member Author

eltigerchino commented Nov 13, 2023

Setting noEmit in the generated file makes sense to me assuming we can do it just for JS projects.

I think it might be needed for TS projects too for whatever reason someone wants to have JS files in their TS project.

I'm not sure about "moduleResolution": "NodeNext" though. Won't that start requiring users to have a .js extension? In SvelteKit 2, I think we'd like to set it to bundler and require TypeScript 5

"moduleResolution": "NodeNext" is only being set for library projects at the moment. I don't much about what module and moduleResolution do.

EDIT: the generated file seems to be exporting moduleResolution: node which apparently means node10? https://www.typescriptlang.org/tsconfig#moduleResolution

@dummdidumm
Copy link
Member

Yeah it is. While we're at it, I think it makes sense to set "moduleResolution": "bundler" in create-svelte for new projects, too. In SvelteKit 2 we can make that the generated default, but to not break things we shouldn't change that in 1.x

@eltigerchino
Copy link
Member Author

While we're at it, I think it makes sense to set "moduleResolution": "bundler" in create-svelte for new projects, too.

Is that for all projects or just the library skeleton?

@eltigerchino eltigerchino changed the title fix: set module: NodeNext and noEmit in tsconfig.json when scaffolding projects fix: set module: bundler and noEmit in tsconfig.json when scaffolding projects Nov 13, 2023
@benmccann
Copy link
Member

benmccann commented Nov 13, 2023

"moduleResolution": "NodeNext" is only being set for library projects at the moment.

Ah, thanks. I missed that. That sounds good to me.

Is that for all projects or just the library skeleton?

I think libraries should be nodenext and applications should be bundler. (I usually see these written all lowercase as the TypeScript docs have them that way). nodenext is stricter and bundler is more permissive. It's best to be strict with libraries so that apps have the easiest time consuming them and permissive with apps so that they can consume the widest range of libraries

@hyunbinseo
Copy link
Contributor

This one is fixed by providing a index.ts file instead of .js in the example lib folder.

This won't help though if you're creating a library using JSDoc, you won't have a .ts file then. This is related to #9552 and the fix is something like this: #9552 (comment)

The reason for proposing index.ts is to make it match the Skeleton project template.

As of create-svelte@5.2.0 there is this inconsistency.

src/lib JSDoc TypeScript
Library index.js index.js *
Skeleton index.js index.ts

The noEmit related issue is mentioned in the o.g. issue.

@eltigerchino eltigerchino changed the title fix: set module: bundler and noEmit in tsconfig.json when scaffolding projects fix: correctly configure module and noEmit tsconfig and jsconfig properties Nov 14, 2023
@dummdidumm dummdidumm merged commit 127bdea into master Nov 14, 2023
14 checks passed
@dummdidumm dummdidumm deleted the fix-create-svelte-lib-ts branch November 14, 2023 12:24
@github-actions github-actions bot mentioned this pull request Nov 14, 2023
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.

Library project tsconfig errors - module value, overwrite input file Svelte kit demo project tsconfig errors
4 participants