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: Use types from @types/bun package #5942

Merged
merged 3 commits into from
Mar 4, 2024
Merged

Conversation

octet-stream
Copy link
Contributor

@octet-stream octet-stream commented Mar 4, 2024

Overview

What is it?

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

Description

This PR removes typings for Bun adapter in favor of the official @types/bun package. This will further simplify the setup for this adapter. I've also updated starters/adapters/bun/src/entry.bun.ts file to rely on that package.

Use cases and why

    1. One use case
    1. Another use case

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

netlify bot commented Mar 4, 2024

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit b5efa8d

@octet-stream
Copy link
Contributor Author

Two things in addition to this PR:

  1. I wasn't sure what @types/bun at the project's root is used for so I kept it there, but I think it can be removed too, because @types/bun (the package) was installed at the root of the monorepo, so it should be available across the project.
  2. I wasn't able to build and test my changes, because neither pnpm build nor pnpm build.full can finish successfully. I was running these two commands within docker container via terminal attached to the container. Is this the right way to run these commands or I misinterpreted the contributing guides?

@octet-stream octet-stream marked this pull request as ready for review March 4, 2024 14:09
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.

LGTM

@wmertens wmertens enabled auto-merge (rebase) March 4, 2024 14:19
@wmertens
Copy link
Member

wmertens commented Mar 4, 2024

@octet-stream you need to run pnpm api.update

auto-merge was automatically disabled March 4, 2024 14:36

Head branch was pushed to by a user without write access

@octet-stream
Copy link
Contributor Author

Yeah, I see. Did the update.

@wmertens
Copy link
Member

wmertens commented Mar 4, 2024

Oh and you should be able to run pnpm build.local. If not, what is the error?

@octet-stream
Copy link
Contributor Author

Was able to successfully run pnpm build.local

@wmertens
Copy link
Member

wmertens commented Mar 4, 2024

I'm really surprised about Loader_2 though

@octet-stream
Copy link
Contributor Author

I have an updated files in docs/routes/qwik (api.json and index.md) after I run this command. Should I commit it?

@octet-stream
Copy link
Contributor Author

Yeah, I have no idea why is it there.

I did in the last commit:

  1. Switched to main branch
  2. Pulled changes from the upstream (with --rebase, it's my default)
  3. Switched to bun-types
  4. Merged main into my branch
  5. Run pnpm api.update inside a docker container

@wmertens wmertens merged commit ea6ea9a into QwikDev:main Mar 4, 2024
22 checks passed
@wmertens
Copy link
Member

wmertens commented Mar 4, 2024

ok, thanks!

@octet-stream octet-stream deleted the bun-types branch March 4, 2024 15:11
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.

None yet

3 participants