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 option to force enable TypeScript option checkJs #352

Closed
4 tasks done
remcohaszing opened this issue Nov 9, 2023 · 7 comments · Fixed by #387
Closed
4 tasks done

Add option to force enable TypeScript option checkJs #352

remcohaszing opened this issue Nov 9, 2023 · 7 comments · Fixed by #387
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🧒 semver/minor This is backwards-compatible change 🦋 type/enhancement This is great to have

Comments

@remcohaszing
Copy link
Member

Initial checklist

Problem

Under the hood MDX is transformed to JavaScript(X) for type checking. By default TypeScript only checks TypeScript(X). It provides subtle editor features, but no red squiggly lines for type errors. Some users expect the latter.

Solution

Add a configuration option to force more strict type checking inside MDX files.

Alternatives

N/A

@remcohaszing remcohaszing added 🦋 type/enhancement This is great to have 🗄 area/interface This affects the public interface 🧒 semver/minor This is backwards-compatible change 🤞 phase/open Post is being triaged manually labels Nov 9, 2023
@remcohaszing
Copy link
Member Author

I see two possible implementations:

  1. Have the option enable checkJs in TypeScript compiler settings.
  2. Inject a // @ts-check comment in the virtual JavaScript file.

1 is simpler, but 2 feels more elegant. 2 requires some changes to how the virtual documents are built, but I’ve been wanting to change that anyway.

Also we need to come up with a name for the configuration setting. I consider the use of allowJs / checkJs an implementation detail. mdx.check? mdx.strict? mdx.validate? mdx.typecheck? mdx.squigglies?

cc @johnsoncodehk

@wooorm
Copy link
Member

wooorm commented Nov 10, 2023

What if users do {/* @ts-check */}? Does it work/would it work?

@remcohaszing
Copy link
Member Author

That doesn’t work. It must be a line comment at the top of the file. In case of Volar at the top of the virtual JS file.

This does work.

{
// @ts-check
}

We could make {/* @ts-check */} work.

We could also make a custom setting in tsconfig.json work, for example:

{
  "compilerOptions": {},
  "mdx": {
    // We already have this
    "plugins": [],
    // This would be new
    "check": true
  }
}

Regardless, I think it’s good to allow the user to control this setting in their editor configuration. It’s a preference. People might not want to commit changes just to enhance the MDX editing experience.

@lea255ace
Copy link

This would be a great addition!
In the meantime I found a workaround with tsconfig.json to get the desired type checking behavior:

{
  "compilerOptions": {
    "checkJs": true,
  },
  "include": {
    "**/*.mdx"
  }
}

@johnsoncodehk
Copy link
Contributor

Volar 2.0 will recommend based on TS Plugin to improve performance, and TS Plugin cannot get IDE configs, so I recommend solving it through tsconfig option for future implementation.

I think the // @ts-check approach is good, it's less hacky to the language server / tsserver. I recommend options named mdx.generateTsCheck or mdxCompilerOptions.generateTsCheck, but this is just a personal preference.

@remcohaszing
Copy link
Member Author

Related PR: johnsoncodehk#2

It looks like it is possible to pass configuration options to TypeScript plugins from a VSCode extension (https://code.visualstudio.com/api/references/contribution-points#Plugin-configuration).

remcohaszing added a commit that referenced this issue Jan 20, 2024
The MDX language service now accepts the option `checkMdx`. When this
is enabled, it tells TypeScript to type check the virtual JSX by
inserting a `@ts-check` comment. The language server reads this as the
option `mdx.checkMdx` from `tsconfig.json`.

Closes #352
remcohaszing added a commit that referenced this issue Jan 20, 2024
The MDX language service now accepts the option `checkMdx`. When this
is enabled, it tells TypeScript to type check the virtual JSX by
inserting a `@ts-check` comment. The language server reads this as the
option `mdx.checkMdx` from `tsconfig.json`.

Closes #352

This comment has been minimized.

@remcohaszing remcohaszing added the 💪 phase/solved Post is done label Jan 20, 2024
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Jan 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🧒 semver/minor This is backwards-compatible change 🦋 type/enhancement This is great to have
Development

Successfully merging a pull request may close this issue.

4 participants