[go: nahoru, domu]

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

NPM types: TSX warnings about JSX.IntrinsicElements in React #16653

Closed
oscarotero opened this issue Nov 15, 2022 · 15 comments · Fixed by #23419
Closed

NPM types: TSX warnings about JSX.IntrinsicElements in React #16653

oscarotero opened this issue Nov 15, 2022 · 15 comments · Fixed by #23419

Comments

@oscarotero
Copy link

I have a TSX project with the following deno.json configuration:

{
  "compilerOptions": {
    "jsx": "react-jsx",
    "jsxImportSource": "npm:react"
  }
}

In VS Code I see the following warning:

imaxe

This doesn't happen if the JSX import source is preact (npm:preact).

I think the reason is because the preact NPM package includes the types (https://github.com/preactjs/preact/blob/master/src/index.d.ts) so Deno can load them.

React needs to fetch the types from @types/react and this is not implemented in Deno. In the NPM roadmap (#15960) I can see the uncompleted task automatic acquisition of @types package for type checking.

Is there any plans to implement this at anytime soon? I think it should be quite useful. At this moment, the only way to use React in Deno without TSX errors is by importing from esm.sh.

@oscarotero oscarotero mentioned this issue Nov 16, 2022
25 tasks
@bartlomieju
Copy link
Member

It is planned, but currently no ETA. Have you tried adding // @deno-types="npm:@types/react" as an escape hatch?

@oscarotero
Copy link
Author

It doesn't work:
imaxe

Not sure if I have to add this in another place.

@dsherret
Copy link
Member
dsherret commented Nov 16, 2022

Yeah, that wouldn't work because there's no import to set that on.

One way to set the JSX globals is to do:

/// <reference types="npm:@types/react@18" />

...or do...

import type {} from "npm:@types/react@18";

But then I get the error:

'React' refers to a UMD global, but the current file is a module. Consider adding an import instead.

...which only occurs in the LSP... not sure why. I think that's because React from @types/react doesn't get associated to the npm:react import and we probably should resolve it there when a @types/x package exists for x and x has no types.

@dsherret
Copy link
Member
dsherret commented Nov 16, 2022

Bartek's latest commit (300fd07) resolves that issue I was seeing in my last comment (it was incorrectly resolving npm:react to the js file for type checking). Adding a tiple slash directive reference or import type for npm:@types/react@18 seems to be the way to go here in the next patch release. I did come across #16654, which I will look into tomorrow.

@Oyelowo
Copy link
Oyelowo commented Nov 17, 2022

@dsherret
Has this now been resolved in the latest release - 1.28.1?
I'm asking as I am still running into the same issue - code runs but the type-checkings fail as mentioned above.

I'm also getting the below error on all my imports which causes types to fail altogether.
One other source of confusion is the deno-react-template does not use import_map.json at all and all imports are done in vite.config. Which is the way to go?

Screenshot 2022-11-17 at 18 21 10

thanks for your work on this! 👍

@Oyelowo
Copy link
Oyelowo commented Nov 19, 2022

I resolved temporarily this by using import_map with esm.sh whole vite usings npm: mapping for building and running

@sant123
Copy link
sant123 commented Nov 21, 2022

This is my workaround for server side rendering. Hope it helps:

// @deno-types="npm:@types/react"
import React from "npm:react";
// @deno-types="npm:@types/react-dom/server"
import ReactDOM from "npm:react-dom/server";

function App() {
  return <h1>Hello world!</h1>;
}

console.log(ReactDOM.renderToString(<App />));

@jimisaacs
Copy link

renderToString is still resolving as any with the above ☝️ for me. It doesn't seem like // @deno-types="npm:@types/react-dom/server" is working as expected.

@dsherret
Copy link
Member

@jimisaacs maybe related #17012 -- this was just fixed and a test was added yesterday to prevent regressions. It seems to work in canary (deno upgrade --canary).

@jimisaacs
Copy link
jimisaacs commented Dec 16, 2022

@dsherret Nice! Good to know it seems to be addressed. Though unfortunately it's a little difficult to try because of the following error: Canary builds are not available for M1

I was hoping to see if all this was related, i.e. if the same issue was causing the following error with deno test (interestingly this error is not a problem with deno run):

error: Uncaught Error: Error resolving package config for 'npm:@types/react-dom@18/server'.: [ERR_INVALID_PACKAGE_TARGET] Invalid "exports" target {"default":"./server.d.ts"} defined for './server' in the package config /my-deno-dir/cache/npm/registry.npmjs.org/@types/react-dom/18.0.9package.json imported from file:///my-deno-dir/cache/npm/registry.npmjs.org/@types/react-dom/18.0.9/; target must start with "./"

@dsherret
Copy link
Member

That's too bad. I wish we had canary builds with M1. We need an M1 CI to do that, but github doesn't have them yet.

Yeah, you can bypass that error by skipping type checking with deno test by doing deno test --no-check (deno run does not type check by default). It will be fixed in the next patch release.

@jimisaacs
Copy link

🙇 --no-check did the trick, like 99.9% confident it's all the same issue. Thank you!

@sant123
Copy link
sant123 commented Dec 16, 2022

@dsherret @jimisaacs just tested with canary and looks good!!

image

@dylanblokhuis
Copy link

This will also work:

import type {} from "npm:@types/react"
import type {} from "npm:@types/react-dom"

The LSP will correctly pickup the types

@nayeemrmn
Copy link
Collaborator

Ref #20455

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants