Skip to content

Rework SDK and user tsconfig.json for optimal DX #2656

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

Merged
merged 33 commits into from
Apr 23, 2025
Merged

Conversation

sodic
Copy link
Contributor

@sodic sodic commented Apr 14, 2025

Fixes #2258
Fixes #2259
Fixes #1986

I commented all the changes and tested it with waspc/examples/todoApp (both in dev and production).

Todo:

Offspring issues with future work:

Copy link
Contributor Author

@sodic sodic left a comment

Choose a reason for hiding this comment

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

Left some elaborating comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of moving these elaborating comments to Generator/ExternalConfig/TsConfig.hs.

Pros:

  • User TS config is less noisy.
  • I can write longer comments.

Cons:

  • Curious users won't get insight into why we choose specific properties.

@infomiho @cprecioso What do you guys think?

Copy link
Member

@cprecioso cprecioso Apr 15, 2025

Choose a reason for hiding this comment

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

No strong opinion, IIUC users are not expected/allowed to modify the tsconfig, so it shouldn't matter. If foresee users able to modify some keys, then I'd keep the comments on those.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the majority of users read this config file and its comments. I'm okay with moving the comments into Haskell - for our sake so we know why we did what we did.

One extra option be to move the comments and then link to the Haskell file on github.com where the comments live for the curious power users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved them to Haskell but didn't include the link to the Haskell source. I don't think it's necessary after all

"esModuleInterop": true,
"moduleResolution": "node",
"outDir": "dist",
"skipLibCheck": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No change here, skipLibCheck was just moved.

"compilerOptions": {
/* Basic opitions */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kicked out the "extends": "@tsconfig/node{= majorNodeVersion =}/tsconfig.json":

Copy link
Contributor

Choose a reason for hiding this comment

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

Great change, we are more self reliant and we don't have to worry about version bumps causing weird side effects. We do need to keep in mind to stay up to date with the best practices for the tsconfig.json over time 👍

Comment on lines 13 to 14
"rootDir": ".",
"outDir": "dist",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No change here:

  • rootDir was implicitly set to ".". I just made it explicit.
  • outDir field was just moved.

// Needed because this is used as a project reference.
"composite": true,
"target": "esnext",
"target": "es2022",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed from es2017 to es2022. We're controlling the entire environment, so I set the this to the newest possible non-moving compilation target.

Copy link
Member

Choose a reason for hiding this comment

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

I'd actually say here esnext because we don't want TypeScript to be handling downleveling the code at all, right? Since Vite is the one that will be handling it for us, we don't want TypeScript to be complaining about it.

Copy link
Contributor Author

@sodic sodic Apr 14, 2025

Choose a reason for hiding this comment

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

You are right, yes. I'll change it up :)
I'll do the same thing for module as well (since TS is the one processing this code before the bundler).

Comment on lines 96 to 102
/*
The `incremental` option enables faster SDK compilation, less Vite HMR
messages and it prevent unnecessary full-page reloads when using Vite HMR.
While this is great, we still want to dig deeper at some point to understand
better why Vite HMR misbehaves when the SDK is recompiled:
https://github.com/wasp-lang/wasp/issues/1934
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only formatting.

"rootDir": ".",
"outDir": "dist",

/* Strictness and type checking */
Copy link
Contributor Author

@sodic sodic Apr 14, 2025

Choose a reason for hiding this comment

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

To the reviewers: you should be able to understand all the fields/decisions from either:

  • The field's name
  • The in-file comments.
  • The mentioned issues.

If you don't, then the comments aren't good enough :)

So please let me know what I should add!

Comment on lines 23 to 25
// Prevent features that don't work with transpilers (e.g., const enums)
"isolatedModules": true,
"moduleDetection": "force",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only changed module and target and added isolatedModules and moduleDetection.

I could write a longer comment here (but it's probably too long to keep in user's tsconfig.json). One more reason to move them to Haskell.

Check this out for the changes in this file: https://www.totaltypescript.com/tsconfig-cheat-sheet

Copy link
Member

Choose a reason for hiding this comment

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

TypeScript recently shipped a flag that includes these changes: erasableSyntaxOnly

https://devblogs.microsoft.com/typescript/announcing-typescript-5-8/#the---erasablesyntaxonly-option

Copy link
Contributor Author

@sodic sodic Apr 14, 2025

Choose a reason for hiding this comment

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

Hm, I belive that option does more stuff than just being a shorthand for these two fields (e.g., it removes enums altogether).

We need isolatedModules to correctly model our code's current behavior. TS currently doesn't warn about these casees and they break in runtime.
That's not the case for erasableSyntaxOnly so I wouldn't force it on our users yet (our code works fine with non-erasable syntax).

The erasableSyntaxOnly setting also typically relies on verbatimModuleSyntax being set to true, which I'm also not sure I'd do right away (since it forces type imports/exports). Let's discuss that one here: #2657

Copy link
Contributor

Choose a reason for hiding this comment

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

You say

I only changed module and target

but I don't see those changes in this diff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed them back based on this comment: #2656 (comment)

"compilerOptions": {
/* Basic opitions */
"target": "es2022",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same story as in skeleton/tsconfig.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a copy of the new skeleton/tsconfig.json.

Comment on lines 25 to 33
"strict": true,
"useUnknownInCatchVariables": false,
"noImplicitAny": false,
"strictPropertyInitialization": false,
// The following two properties are disabled because we don't
// yet want to force user code to use them.
// Reference: https://github.com/wasp-lang/wasp/issues/2655
"noUncheckedIndexedAccess": false,
"noImplicitOverride": false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no meaningful changes here, I just flipped things around. Instead of setting strict to false and then activate the flags can use, we now set strict to true and deactivate the flags we can't use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't noUncheckedIndexedAccess and noUncheckedIndexedAccess false by default? Even with strict?
Is this more of a documentation to signal we want to change those in future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't noUncheckedIndexedAccess and noUncheckedIndexedAccess false by default?

They are, yes

Is this more of a documentation to signal we want to change those in future?

Also yes. I could remove them though since we have the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

You have more context about that here. Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept them in the end.

Comment on lines 36 to 38
"declaration": true, // Allows users to consume the SDK as a library, enables autocomplete
"declarationMap": true, // Makes go-to-defintion go to source instead of type definitios
"sourceMap": true, // Needed for debugging
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sourceMap and declaration are just moved. declarationMap is new and fixes #1986

Comment on lines 23 to 25
// Prevent features that don't work with transpilers (e.g., const enums)
"isolatedModules": true,
"moduleDetection": "force",
Copy link
Member

Choose a reason for hiding this comment

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

TypeScript recently shipped a flag that includes these changes: erasableSyntaxOnly

https://devblogs.microsoft.com/typescript/announcing-typescript-5-8/#the---erasablesyntaxonly-option

// Needed because this is used as a project reference.
"composite": true,
"target": "esnext",
"target": "es2022",
Copy link
Member

Choose a reason for hiding this comment

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

I'd actually say here esnext because we don't want TypeScript to be handling downleveling the code at all, right? Since Vite is the one that will be handling it for us, we don't want TypeScript to be complaining about it.

Comment on lines 42 to 43
// NOTE: Overriding `typeRoots` prevents TS from climbing up the file
// tree and including `node_modules/@types` from parent folders.
Copy link
Member

Choose a reason for hiding this comment

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

Mmm I'd try to add
import "@testing-library/jest-dom"; in vite-env.d.ts and I think we'd avoid doing stuff in typeRoots

@Martinsos Martinsos requested a review from infomiho April 14, 2025 14:18
Since we can't have "bundler", "node10" is the next best thing.
Source: https://www.typescriptlang.org/docs/handbook/modules/theory.html#module-resolution

TODO: Change "moduleResolution" to "bundler" when we move away from stitches.
Copy link
Contributor

@infomiho infomiho Apr 15, 2025

Choose a reason for hiding this comment

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

I'd bump the priority on moving away from Stitches #1565

/*
User code was compiled uses `"strict": true`, so we know we can use it
here as well. We override some of strict's options because parts of the SDK
code don't respec them. This only makes the SDK ruleset more permissive than
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
code don't respec them. This only makes the SDK ruleset more permissive than
code don't respect them. This only makes the SDK ruleset more permissive than

Comment on lines 29 to 31
// The following two properties are disabled because we don't
// yet want to force user code to use them.
// Reference: https://github.com/wasp-lang/wasp/issues/2655
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Let's use block comment style to make the comment style uniform.

// This is needed to properly support Vitest testing with jest-dom matchers.
// Types for jest-dom are not recognized automatically and Typescript complains
// about missing types e.g. when using `toBeInTheDocument` and other matchers.
import '@testing-library/jest-dom'
Copy link
Contributor

@infomiho infomiho Apr 15, 2025

Choose a reason for hiding this comment

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

This is a "breaking change" for users to be able to use Vitest again? Our starter templates should include this line as well then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -17,26 +17,25 @@
// We're bundling all code in the end so this is the most appropriate option,
Copy link
Contributor

@infomiho infomiho Apr 15, 2025

Choose a reason for hiding this comment

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

It would be nice if we could come up with a way for Wasp CLI to "upgrade" the tsconfig.json when it's broken for users e.g. set the correct values after validation catches issues. That would make these kind of changes much more palatable. It's not terrible, but maybe we can play around with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting idea, care to create a mr. issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will do 👍

@infomiho
Copy link
Contributor

I love the jump to definition behavior change 👍

@FranjoMindek FranjoMindek added refactoring Keeping that code clean! dx labels Apr 16, 2025
@@ -3,12 +3,27 @@
## Unreleased

### ⚠️ Breaking Changes
Address all breaking changes by following [the official migration guide](https://wasp.sh/docs/migration-guides/migrate-from-0-16-to-0-17). This is just a short overview:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will become a valid link once we release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Address all breaking changes by following [the official migration guide](https://wasp.sh/docs/migration-guides/migrate-from-0-16-to-0-17). This is just a short overview:
Follow the [the official migration guide](https://wasp.sh/docs/migration-guides/migrate-from-0-16-to-0-17) to address all the breaking changes. Here's a short overview:

```

1. Go into the Wasp project you want to switch to the Wasp TS config (or create a new Wasp project if you want to try it out like that). Make sure you are on Wasp >= 0.16.3 and your project is working.
2. Rename `tsconfig.json` file to `tsconfig.src.json`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasp >= 0.16.0 validates their configs have include properly set up.

:::caution Requires Wasp >= 0.15
This document assumes your app works with Wasp >= 0.15.
If you haven't migrated your app yet, follow the [migration instructions](../migration-guides/migrate-from-0-14-to-0-15.md) and verify everything works. After that, come back here and try out the new Wasp TS config.
:::caution Requires Wasp >= 0.16.3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I bumped this up since we made a change in the public API with 0.16.3 (HttpRoute by @FranjoMindek).

Comment on lines +42 to +68
-- References for understanding the required compiler options:
-- - The comments in templates/sdk/wasp/tsconfig.json
-- - https://www.typescriptlang.org/docs/handbook/modules/introduction.html
-- - https://www.totaltypescript.com/tsconfig-cheat-sheet
-- - https://www.typescriptlang.org/tsconfig/
concat
[ validateRequiredFieldInCompilerOptions "module" T._module "esnext",
validateRequiredFieldInCompilerOptions "target" T.target "esnext",
-- Since Wasp ends up bundling the user code, `bundler` is the most
-- appropriate `moduleResolution` option.
validateRequiredFieldInCompilerOptions "moduleResolution" T.moduleResolution "bundler",
validateRequiredFieldInCompilerOptions "moduleDetection" T.moduleDetection "force",
-- `isolatedModules` prevents users from using features that don't work
-- with transpilers and would fail when Wasp bundles the code with rollup
-- (e.g., const enums)
validateRequiredFieldInCompilerOptions "isolatedModules" T.isolatedModules True,
validateRequiredFieldInCompilerOptions "jsx" T.jsx "preserve",
validateRequiredFieldInCompilerOptions "strict" T.strict True,
validateRequiredFieldInCompilerOptions "esModuleInterop" T.esModuleInterop True,
validateRequiredFieldInCompilerOptions "lib" T.lib ["dom", "dom.iterable", "esnext"],
validateRequiredFieldInCompilerOptions "allowJs" T.allowJs True,
validateRequiredFieldInCompilerOptions "typeRoots" T.typeRoots ["node_modules/@testing-library", "node_modules/@types"],
-- Wasp internally uses TypeScript's project references to compile the
-- code. Referenced projects may not disable emit, so we must specify an
-- `outDir`.
validateRequiredFieldInCompilerOptions "outDir" T.outDir ".wasp/out/user",
-- The composite flag is required because Wasp uses project references
-- (i.e., web app and server reference user code as a subproject)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These comments were moved from user's tsconfig.json and expanded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved them to Haskell but didn't include the link to the Haskell source. I don't think it's necessary after all

@sodic sodic force-pushed the filip-sdk-tsconfig branch from 725aaea to a22ad9f Compare April 17, 2025 00:03
Copy link
Member

@cprecioso cprecioso left a comment

Choose a reason for hiding this comment

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

LGTM, leaving it approved, added some nitpicky comments about phrasing but code-wise is good

Comment on lines 11 to 20
```json
{
"compilerOptions": {
// ...
"moduleDetection": "force",
"isolatedModules": true,
// Remove 'typeRoots'
},
}
```
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather express this as a diff, it renders nicely:

Suggested change
```json
{
"compilerOptions": {
// ...
"moduleDetection": "force",
"isolatedModules": true,
// Remove 'typeRoots'
},
}
```
```diff
{
"compilerOptions": {
+ "moduleDetection": "force",
+ "isolatedModules": true,
- "typeRoots": [ ... ]
},
}
```


- In the `usernameAndPassword` authentication method, the function `login()` imported from `wasp/client/auth` now accepts an object with `username` and `password` instead of two separate arguments ([#2598](https://github.com/wasp-lang/wasp/pull/2598))
- We've made some improvements to our TypeScript setup that require you to update the `tsconfig.json` file. The migration guide will lead you through them.
For posterity, here are all the non-cosmetic changes we've made:
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: everything in the changelog is for posterity 😂

Suggested change
For posterity, here are all the non-cosmetic changes we've made:
Here are the non-cosmetic changes we made:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, fair enough!

},
}
```
- [The issue](https://github.com/testing-library/jest-dom/issues/546#issuecomment-1889884843) with `jest-dom` types is now addressed in `src/vite-env.d.ts`.
Copy link
Member

Choose a reason for hiding this comment

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

This entry is not very understandable for users I think, especially since it links to an issue in another project altogether, and the error it describes is not one our users were having (because we were already solving it).

I'd go for something along the lines of

Suggested change
- [The issue](https://github.com/testing-library/jest-dom/issues/546#issuecomment-1889884843) with `jest-dom` types is now addressed in `src/vite-env.d.ts`.
- The types for DOM helpers in tests are now imported from `src/vite-env.d.ts` instead of changing the `tsconfig.json`'s `typeRoots`.

(and I'm sure it can be improved further 😅)


### 🔧 Small improvements

- Show a friendlier error when there are no routes defined in the wasp file ([#2643](https://github.com/wasp-lang/wasp/pull/2643))
- Better TypeScript support ([#2656](https://github.com/wasp-lang/wasp/pull/2656))
Copy link
Member

Choose a reason for hiding this comment

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

Let's be specific

Suggested change
- Better TypeScript support ([#2656](https://github.com/wasp-lang/wasp/pull/2656))
- Modernized our TypeScript configuration and added support for jump-to-definition ([#2656](https://github.com/wasp-lang/wasp/pull/2656))

// Types for jest-dom are not recognized automatically and Typescript complains
// about missing types e.g. when using `toBeInTheDocument` and other matchers.
// Reference: https://github.com/testing-library/jest-dom/issues/546#issuecomment-1889884843
import '@testing-library/jest-dom'
Copy link
Member

Choose a reason for hiding this comment

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

letss gooooo 🔥

"compilerOptions": {
/* Basic opitions */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* Basic opitions */
/* Basic options */

@@ -104,7 +104,68 @@ It is possible that you were not using this function in your code.
If you're instead using [the `<LoginForm>` component](../auth/ui.md#login-form),
this change is already handled for you.

### 2. Enjoy your updated Wasp app
### Update your `tsconfig.json`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### Update your `tsconfig.json`
### 2. Update your `tsconfig.json`

new version of the file and reapplying them.

Here's the new version `tsconfig.json`:
```json title=tsconfig.json
Copy link
Member

Choose a reason for hiding this comment

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

I seem to remember you can use ```jsonc so that comments are supported? Maybe you can try it

Copy link
Contributor Author

@sodic sodic Apr 23, 2025

Choose a reason for hiding this comment

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

Tried it, doesn't work. But json renders comments normally in docusaurus so it doesn't matter:
image

the case for most users), just replace its contents with the new version shown
below.

If you have made changes to your `tsconfig.json` file, we recommend taking the
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be really awesome if we could "fix" the errors in the tsconfig for the users. I know I'm repeating myself but I'm lobbying for the change at some point in the future 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll repeat myself too :D

#2656 (comment)

```

### 3. Tell Wasp about `jest-dom` types
If you're using (or planning to use) Wasp's [client tests](../project/testing.md) with `jest-dom`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If you're using (or planning to use) Wasp's [client tests](../project/testing.md) with `jest-dom`,
If you're using Wasp's [client tests](../project/testing.md) with `jest-dom`,

IMHO no need to anticipate every scenario with the sentence

Copy link
Contributor Author

@sodic sodic Apr 23, 2025

Choose a reason for hiding this comment

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

I decided to keep it. I'd like to scare the reader into thinking "Well, I'm not using it but I don't want to give up on the idea" and doing the update. We'll have fewer discord support questions this way.

Comment on lines 160 to 166
```ts
// This is needed to properly support Vitest testing with jest-dom matchers.
// Types for jest-dom are not recognized automatically and Typescript complains
// about missing types e.g. when using `toBeInTheDocument` and other matchers.
// Reference: https://github.com/testing-library/jest-dom/issues/546#issuecomment-1889884843
import '@testing-library/jest-dom'
```
Copy link
Contributor

Choose a reason for hiding this comment

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

For better copy pastebility, I'd put the full contents they can just copy paste (99% users didn't change their vite-env.d.ts file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice one.

@sodic sodic merged commit db7c774 into main Apr 23, 2025
6 checks passed
@sodic sodic deleted the filip-sdk-tsconfig branch April 23, 2025 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dx refactoring Keeping that code clean!
Projects
None yet
4 participants