denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
97.48k stars 5.38k forks source link

Bug: linting + lsp errors when `jsx` is set to `preserve` #25677

Open marvinhagemeister opened 1 month ago

marvinhagemeister commented 1 month ago

Setting the jsx compiler option to preserve leads to an error in the LSP and when running deno lint. The preserve option is a valid value though.

Steps to reproduce:

  1. Create a deno.json with these contents:
    {
    "compilerOptions": {
      "jsx": "preserve",
      "jsxImportSource": "preact"
    },
    "imports": {
      "preact": "npm:preact@^10.24.0"
    }
    }
  2. Open the folder containing the deno.json in vscode.

Output:

Error caching: Unsupported 'jsx' compiler option value 'preserve'. Supported: 'react-jsx', 'react-jsxdev', 'react', 'precompile'
  at file:///Users/marvinh/dev/test/deno-jsx-preservec/deno.json
Screenshot 2024-09-17 at 10 51 22

Version: Deno 2.0.0-rc.2+aaf2bf4

lucacasonato commented 1 month ago

When we pass the config to TSC, we should turn preserve into react-jsx.

marvinhagemeister commented 1 month ago

Updated the title as this also affects deno lint

lucacasonato commented 1 month ago

Actually - no, we don't support preserve anywhere. In the context of Deno, that option makes no sense because V8 would not be able to execute preserved JSX in a file. We always have to transpile it.

marvinhagemeister commented 1 month ago

Yeah, but when you use Deno in a vite project, the code will be transpiled by vite before handing it off to Deno.

lucacasonato commented 1 month ago

But for Deno (which is what the deno.json is configuring), "preserve" never makes sense

marvinhagemeister commented 1 month ago

But we need to configure it there for the LSP.

lucacasonato commented 1 month ago

Not in the deno.json? The "preserve" option for purposes of type checking behaves the same in the LSP as "react", no?

birkskyum commented 1 month ago

Not in the deno.json? The "preserve" option for purposes of type checking behaves the same in the LSP as "react", no?

In react/react-native code maybe, but is it also true for solid-js and other non-react jsx implementations?

lucacasonato commented 1 month ago

Yes - type checking for JSX works the same in solid and react

dsherret commented 1 month ago

This is currently intended behaviour and not a bug. Probably we should improve the error message to explain why.

lucacasonato commented 1 month ago

I propose we enhance the error message for "preserve", changing it to:

Unsupported "jsx" compiler option value "preserve". Deno executes TypeScript instead of transpiling it to disk, so preserving JSX does not make sense. If you are setting "preserve" for a frontend framework like Solid, use "react-jsx" instead.

marvinhagemeister commented 1 month ago

Disagree, we should support the preserve option or set it to react-jsx behind the scenes. There shouldn't be any worning. Not every JSX transpilation goes through us when bundlers are used. Setting it to react-jsx makes no sense for a SolidJS user.

dsherret commented 1 month ago

But the compiler options in a deno.json are for Deno and not a bundler. I like the idea of enhancing the error message and not supporting a preserve mode that's not really preserve.

nayeemrmn commented 1 month ago

Setting it to react-jsx makes no sense for a SolidJS user.

Hmm.. from playing around with SolidJS templates and userland projects it seems like they are a bit misguided in suggesting { "jsx": "preserve" }. They should be using { "jsx": "react-jsx", "noEmit": true } which works fine. They don't rely on tsc's transpiler at all, why bother telling tsc to stop at .tsx -> .jsx transpilation?

That's what preserve does after all. In spirit it seems meant for bundlers that can handle file.jsx but not file.tsx, and makes tsc do only that half of the work. But bundlers like the ones solid works with all support TS.

SolidJS's starter templates seem to use noEmit: true, rendering jsx: preserve meaningless.

birkskyum commented 1 month ago

@nayeemrmn , if there are better ways to handle typescript jsx for solid in deno than how it's done normally, I'm sure the community is open to suggestions. It's a bit cursed if typescript has hardcoded some generic jsx behavior as "react" (did I get that right?), but I don't know if it's deno's responsibility to clean up - in general I'm not sure to what extend deno.json compilerOptions aims aligns with tsconfig. I guess the pragmatic pov is that it would potentially be convenient for people working with solid if they don't have to think about changing the jsx default settings, even if it's called "react" under the hood for historical reasons.

nayeemrmn commented 1 month ago

To be clear I'm not 100% sure that preserve is the same as react-jsx for typechecking, but it's got to align with at least one of the other options right? I guess I wouldn't be too surprised if there was some set of unspecified differences for preserve.