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(template-preact-ts): add preact compat paths to tsconfig #12027

Closed
wants to merge 1 commit into from
Closed

fix(template-preact-ts): add preact compat paths to tsconfig #12027

wants to merge 1 commit into from

Conversation

reifiedbeans
Copy link

Description

This patch adds React compatibility types to template-preact-ts so that new projects using create-vite with Preact and TypeScript include these by default.

Additional context

I ran into some typing issues trying to use react-redux with this Preact template.

Type 'Element' is not assignable to type 'ReactNode'. Property 'children' is missing in
type 'VNode<any>' but required in type 'ReactPortal'.

Preact's docs specify that you can add some type paths to add compatibility for Preact with libraries meant for React.

Your project could need support for the wider React ecosystem. To make your application compile, you might need to disable type checking on your node_modules and add paths to the types like this. This way, your alias will work properly when libraries import React.

Many people use Preact as a faster, lightweight, and compatible alternative to React, so most developers are likely going to use libraries meant for React with Preact. This change removes friction, with what seems to be little added cost, so that developers can quickly get started on a new project.

I'm also happy to turn this PR into a discussion thread if this change is unwanted. (Also, thanks for making Vite; I've never been able to spin up a new project with little overhead this easily before!)


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@sapphi-red
Copy link
Member

For other reviewers: @preact/preset-vite aliases react to preact
https://github.com/preactjs/preset-vite/blob/3ed6fb180ed3845f26281675e45f06c1acbce906/src/index.ts#L174-L180

@sapphi-red sapphi-red added the p2-edge-case Bug, but has workaround or limited in scope (priority) label Feb 13, 2023
@bluwy
Copy link
Member

bluwy commented Feb 13, 2023

I'm a bit worried that this would bring questions of Vite being impartial to package managers that don't use node_modules, like yarn pnp or Deno default mode. Does the error happen even with skipLibCheck: true? Usually that means ignoring errors in libraries, so I'm confused why the paths is still needed when the error is ignored.

@sapphi-red
Copy link
Member

That's a good point. Did a bit of search and found this conversation. #7726 (comment)
Maybe we should just add a link to these issues because it doesn't seem to have a good solution yet.

@reifiedbeans
Copy link
Author

Thanks for the feedback; I can see that this could be an issue for other package managers. While messing around with my project, trying new libraries, I found that the issue was gone once I uninstalled react-redux. Seems that it is pulling in @types/react, which was the source of the issues.

At this point, I think it makes the most sense for react-redux to stop forcing a peer dependency on @types/react. If that isn't feasible, it probably is best for Vite users to add this on a need-by-need basis.

Feel free to close this PR if you think the discussion is resolved.

@bluwy
Copy link
Member

bluwy commented Feb 14, 2023

Maybe we should just add a link to these issues because it doesn't seem to have a good solution yet.

Yeah I think it would be nice to add some comments to the tsconfig to explain about skipLibCheck, and what to do if it's disabled. Closing this for now as that could be done together in #7867.

@bluwy bluwy closed this Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants