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(starters): Fix vite types referencing in base starter #6185

Merged
merged 3 commits into from
May 1, 2024

Conversation

octet-stream
Copy link
Contributor

@octet-stream octet-stream commented Apr 30, 2024

Closes #6059

Overview

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests / types / typos

Description

It's been about a month since this PR was opened and to this point I doubt that it is going somewhere, so I decided to take over and fix this issue, using the same solution I have proposed here, and I was able to verify it is working.

Use cases and why

My previous attempt to fix types for Bun turned out to be incomplete, so this PR fixes the issue. Here's a brief list of changes:

  • Move vite/client types referencing to a separate file called qwik.env.d.ts. This file can be used for more global typings in a future, if needed.
  • Remove types field in compilerOptions for base app starter with a reference to that file in includes field instead.

As I said before, the types field narrows the list of the sources where TypeScript will look for typings for global scope, so I don't think it would be a good fit when there's many other typings that can be needed from node_modules/@types. In our case, we need to support both @types/node and @types/bun both of which has global types. See documentation for more info: https://www.typescriptlang.org/tsconfig#types

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

Copy link
Member

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

I like the idea of not pinning types, but can you add a comment to that effect in your new qwik.env file?

starters/apps/base/tsconfig.json Outdated Show resolved Hide resolved
@octet-stream octet-stream marked this pull request as ready for review April 30, 2024 13:33
@PatrickJS PatrickJS dismissed wmertens’s stale review May 1, 2024 00:08

comment was added

@PatrickJS PatrickJS merged commit f62c103 into QwikDev:main May 1, 2024
22 checks passed
@octet-stream octet-stream deleted the fix/types branch May 1, 2024 08:08
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 this pull request may close these issues.

3 participants