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

feat: Bun Adapter #5129

Merged
merged 10 commits into from
Sep 18, 2023
Merged

feat: Bun Adapter #5129

merged 10 commits into from
Sep 18, 2023

Conversation

EamonHeffernan
Copy link
Contributor

@EamonHeffernan EamonHeffernan commented Sep 8, 2023

Overview

What is it?

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

Description

Adds an adapter for use with Bun as it is being released in v1.0 today.
Bun's package manager cannot run Vite at the moment, therefore npm must be used to install, but bun can be used to run the dev and production servers.

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

Sorry, something went wrong.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@netlify
Copy link

netlify bot commented Sep 8, 2023

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 44a4c84

@EamonHeffernan EamonHeffernan changed the title Bun Adapter feat: Bun Adapter Sep 8, 2023
@birkskyum
Copy link
Contributor

birkskyum commented Sep 8, 2023

Bun's package manager cannot run Vite at the moment

What specifically is the issue here? Is there an open ticket in the Bun repo?

@EamonHeffernan
Copy link
Contributor Author

EamonHeffernan commented Sep 8, 2023

Bun's package manager cannot run Vite at the moment

What specifically is the issue here? Is there an open ticket in the Bun repo?

That might have been unclear sorry, if the packages are installed via bun install running bun run dev fails due to this issue with sharp lovell/sharp#3511. If they are installed via npm install, bun run dev works fine.

Thinking about it, its likely not a vite issue, and just an issue with the specific dependencies of Qwik.

I believe it is being tracked by this issue. oven-sh/bun#3783

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@zanettin zanettin added COMP: starters STATUS-2: PR waiting for review This PR is waiting for review and approval before merge labels Sep 8, 2023
@mhevery
Copy link
Contributor

mhevery commented Sep 8, 2023

Thank you for your contribution. Let us know when you think it is ready or if you want to review a specific area.

@EamonHeffernan
Copy link
Contributor Author

Thank you for your contribution. Let us know when you think it is ready or if you want to review a specific area.

Should be good to go. Just not sure on whether I needed to add those type defs, it worked for me including cli.
It's very basic, doesn't use any of bun's special build features which I assume would take work on vites part.

Copy link
Contributor

@zanettin zanettin left a comment

Choose a reason for hiding this comment

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

Hi @EamonHeffernan
Just went over the code and left a short question. Thanks a lot for this addition 🙏


Qwik City Bun middleware allows you to hook up Qwik City to a Bun server which uses the Bun Http API.

## Installation
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we provide a link to the install guide of bun?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I've added much more detailed information about both the installation and the current issue with using bun install.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@zanettin
Copy link
Contributor

thank u very much for the changes on the docs @EamonHeffernan 🙏
wanted to test run the changes but maybe my setup was not accurate. can you quickly tell me how you've tested it?
in the best case we'd be able to test from bun qwik add bun on the whole flow 👍 thx again

@EamonHeffernan
Copy link
Contributor Author

EamonHeffernan commented Sep 13, 2023

thank u very much for the changes on the docs @EamonHeffernan 🙏 wanted to test run the changes but maybe my setup was not accurate. can you quickly tell me how you've tested it? in the best case we'd be able to test from bun qwik add bun on the whole flow 👍 thx again

My flow for getting it working was build with the docker environment, npm install on a new qwik project, copy dist for qwik and qwik-city to replace the existing folders for qwik and qwik city, then run bun run qwik add bun. Happy to work out what's going on if you can't recreate it.

I also had to do sudo chmod +x node_modules/.bin/qwik which presumably is handled by the install process normally.

@gioboa gioboa linked an issue Sep 14, 2023 that may be closed by this pull request
@zanettin
Copy link
Contributor

Hi @EamonHeffernan 👋
Thanks again for your help 🙏 looks like (at least me) have issues with pnpm and the described flow. I am able to follow these steps when using npm. i've attached the errors below. is this reproduceable on your side?
Bildschirmfoto 2023-09-14 um 22 28 09

and a tiny cosmetic thing: can we adjust the formatting of the CLI output? the box does not fit well atm and shouldn't the listed cmd bun build.server be bun run build.server in New scripts added instead? 👼
Bildschirmfoto 2023-09-14 um 22 15 57

Thanks once again for this fantastic work 🙏 really really appreciated!

@EamonHeffernan
Copy link
Contributor Author

Hi @EamonHeffernan 👋 Thanks again for your help 🙏 looks like (at least me) have issues with pnpm and the described flow. I am able to follow these steps when using npm. i've attached the errors below. is this reproduceable on your side? Bildschirmfoto 2023-09-14 um 22 28 09

and a tiny cosmetic thing: can we adjust the formatting of the CLI output? the box does not fit well atm and shouldn't the listed cmd bun build.server be bun run build.server in New scripts added instead? 👼 Bildschirmfoto 2023-09-14 um 22 15 57

Thanks once again for this fantastic work 🙏 really really appreciated!

Hmm, I haven't seen that issue. My guess on why pnpm doesn't work is that copying the files over might not work as well with its symlinking system, although I might be wrong. My hope would be that this would be resolved upon upload as it looks like when the folder contents are replaced it doesn't properly detect the other packages.

As for the formatting on the CLI, I'm not really sure how to change that, my changes shouldn't of messed with that and I believe it will have been an existing issue.

@zanettin
Copy link
Contributor

thanks again for your feedback 🙏 let me do some final tests and discuss it with the core team.

@EamonHeffernan
Copy link
Contributor Author

Related PR is #5165 that automatically adds a trusted dependency on npm/pnpm/yarn/bun create qwik so that bun as a package manager will work for qwik, regardless of the adapter being used.

Verified

This commit was signed with the committer’s verified signature.
zanettin roman zanettin
@zanettin
Copy link
Contributor

zanettin commented Sep 16, 2023

@EamonHeffernan i've fixed the CLI inputs within your PR. so imo we are good to go with this 🚀
thanks again for your work on this an your patience 🙏

Bildschirmfoto 2023-09-16 um 21 26 12

zanettin and others added 2 commits September 16, 2023 21:30

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was signed with the committer’s verified signature.
zanettin roman zanettin
Copy link
Contributor

@zanettin zanettin left a comment

Choose a reason for hiding this comment

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

thx again 🙏 🎉

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@EamonHeffernan
Copy link
Contributor Author

thx again 🙏 🎉

I've just realised there was something missing due to the changes that went through in the other pr. I've slightly changed the documentation to reflect this.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Contributor

@zanettin zanettin left a comment

Choose a reason for hiding this comment

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

thx for these adjustments as well 🙏

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

For other platforms or if you run into issues with installation, up to date `bun` installation instructions can be found [on the bun website](https://bun.sh/docs/installation).

There currently is an issue with using `bun` as the package manager. If you run into issues using `bun install`, please add the following lines to your package.json file.
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this was resolved. Should it be removed?

@mhevery mhevery merged commit f7585e3 into QwikDev:main Sep 18, 2023
@mhevery
Copy link
Contributor

mhevery commented Sep 18, 2023

Thank you for your hard work and this excellent contribution. ❤️

@EamonHeffernan EamonHeffernan deleted the bun-adapter branch September 18, 2023 19:23
kodiakhq bot referenced this pull request in ascorbic/unpic-img Sep 24, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@builder.io/qwik](https://qwik.builder.io/) ([source](https://togithub.com/BuilderIO/qwik)) | [`1.2.11` -> `1.2.12`](https://renovatebot.com/diffs/npm/@builder.io%2fqwik/1.2.11/1.2.12) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@builder.io%2fqwik/1.2.12?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@builder.io%2fqwik/1.2.12?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@builder.io%2fqwik/1.2.11/1.2.12?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@builder.io%2fqwik/1.2.11/1.2.12?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>BuilderIO/qwik (@&#8203;builder.io/qwik)</summary>

### [`v1.2.12`](https://togithub.com/BuilderIO/qwik/releases/tag/v1.2.12)

[Compare Source](https://togithub.com/BuilderIO/qwik/compare/v1.2.11...v1.2.12)

##### What's Changed

-   docs: interactive resumability Demo component to place on homepage by [@&#8203;mhevery](https://togithub.com/mhevery) in [https://github.com/BuilderIO/qwik/pull/4990](https://togithub.com/BuilderIO/qwik/pull/4990)
-   Insights by [@&#8203;mhevery](https://togithub.com/mhevery) in [https://github.com/BuilderIO/qwik/pull/5146](https://togithub.com/BuilderIO/qwik/pull/5146)
-   docs: add routeAction$ caveat by [@&#8203;gioboa](https://togithub.com/gioboa) in [https://github.com/BuilderIO/qwik/pull/5147](https://togithub.com/BuilderIO/qwik/pull/5147)
-   fix(insight): Limit the size of data retrieved from the database by [@&#8203;mhevery](https://togithub.com/mhevery) in [https://github.com/BuilderIO/qwik/pull/5149](https://togithub.com/BuilderIO/qwik/pull/5149)
-   fix(insight): Limit the size of data retrieved from the database by [@&#8203;mhevery](https://togithub.com/mhevery) in [https://github.com/BuilderIO/qwik/pull/5151](https://togithub.com/BuilderIO/qwik/pull/5151)
-   chore(github-action): added semantic pr title check by [@&#8203;zanettin](https://togithub.com/zanettin) in [https://github.com/BuilderIO/qwik/pull/5152](https://togithub.com/BuilderIO/qwik/pull/5152)
-   fix(github-action): do not validate draft PRs by [@&#8203;zanettin](https://togithub.com/zanettin) in [https://github.com/BuilderIO/qwik/pull/5153](https://togithub.com/BuilderIO/qwik/pull/5153)
-   fix(github-action): do not validate draft PRs - revert by [@&#8203;zanettin](https://togithub.com/zanettin) in [https://github.com/BuilderIO/qwik/pull/5155](https://togithub.com/BuilderIO/qwik/pull/5155)
-   docs: Add expobeds.com to the showcase by [@&#8203;ilianiv](https://togithub.com/ilianiv) in [https://github.com/BuilderIO/qwik/pull/5159](https://togithub.com/BuilderIO/qwik/pull/5159)
-   chore: Update 🐼 PandaCSS integration dev dependency by [@&#8203;mrhoodz](https://togithub.com/mrhoodz) in [https://github.com/BuilderIO/qwik/pull/5156](https://togithub.com/BuilderIO/qwik/pull/5156)
-   feat: leaflet map integration adapter by [@&#8203;anartzdev](https://togithub.com/anartzdev) in [https://github.com/BuilderIO/qwik/pull/5158](https://togithub.com/BuilderIO/qwik/pull/5158)
-   fix: Ignore blob URL on getting image info by [@&#8203;genki](https://togithub.com/genki) in [https://github.com/BuilderIO/qwik/pull/5150](https://togithub.com/BuilderIO/qwik/pull/5150)
-   docs(qwik-city): correct the links to endpoints page by [@&#8203;wtlin1228](https://togithub.com/wtlin1228) in [https://github.com/BuilderIO/qwik/pull/5167](https://togithub.com/BuilderIO/qwik/pull/5167)
-   docs(qwik-city): add validators by [@&#8203;wtlin1228](https://togithub.com/wtlin1228) in [https://github.com/BuilderIO/qwik/pull/5166](https://togithub.com/BuilderIO/qwik/pull/5166)
-   fix: Bun install by [@&#8203;EamonHeffernan](https://togithub.com/EamonHeffernan) in [https://github.com/BuilderIO/qwik/pull/5165](https://togithub.com/BuilderIO/qwik/pull/5165)
-   docs(qwik): Fixed Link by [@&#8203;pipisso](https://togithub.com/pipisso) in [https://github.com/BuilderIO/qwik/pull/5169](https://togithub.com/BuilderIO/qwik/pull/5169)
-   fix(qwik-city): Fix rewrite home route by [@&#8203;claudioshiver](https://togithub.com/claudioshiver) in [https://github.com/BuilderIO/qwik/pull/5168](https://togithub.com/BuilderIO/qwik/pull/5168)
-   docs: use import .css?inline instead of .css by [@&#8203;sapphi-red](https://togithub.com/sapphi-red) in [https://github.com/BuilderIO/qwik/pull/5161](https://togithub.com/BuilderIO/qwik/pull/5161)
-   fix(core.d.ts): export HTMLAttributes and DevJSX to fix TS4023 issue … by [@&#8203;gparlakov](https://togithub.com/gparlakov) in [https://github.com/BuilderIO/qwik/pull/5141](https://togithub.com/BuilderIO/qwik/pull/5141)
-   fix: excludedPath defaults for netlify edge by [@&#8203;adamdbradley](https://togithub.com/adamdbradley) in [https://github.com/BuilderIO/qwik/pull/5163](https://togithub.com/BuilderIO/qwik/pull/5163)
-   fix(labs): filter out (group layouts) by [@&#8203;mhevery](https://togithub.com/mhevery) in [https://github.com/BuilderIO/qwik/pull/5171](https://togithub.com/BuilderIO/qwik/pull/5171)
-   feat(core): added symbol name to error by [@&#8203;shairez](https://togithub.com/shairez) in [https://github.com/BuilderIO/qwik/pull/5182](https://togithub.com/BuilderIO/qwik/pull/5182)
-   feat: Bun Adapter by [@&#8203;EamonHeffernan](https://togithub.com/EamonHeffernan) in [https://github.com/BuilderIO/qwik/pull/5129](https://togithub.com/BuilderIO/qwik/pull/5129)
-   fix(qwik-auth): don't overwrite response headers in qwik auth by [@&#8203;Aslemammad](https://togithub.com/Aslemammad) in [https://github.com/BuilderIO/qwik/pull/5180](https://togithub.com/BuilderIO/qwik/pull/5180)
-   docs(qwik-city): Clean up docs for bun adapter by [@&#8203;EamonHeffernan](https://togithub.com/EamonHeffernan) in [https://github.com/BuilderIO/qwik/pull/5185](https://togithub.com/BuilderIO/qwik/pull/5185)
-   chore(qwik-auth): v0.1.2 by [@&#8203;mhevery](https://togithub.com/mhevery) in [https://github.com/BuilderIO/qwik/pull/5186](https://togithub.com/BuilderIO/qwik/pull/5186)
-   docs: added firebase to menu.md by [@&#8203;the-r3aper7](https://togithub.com/the-r3aper7) in [https://github.com/BuilderIO/qwik/pull/5183](https://togithub.com/BuilderIO/qwik/pull/5183)
-   chore: release 1.2.12 by [@&#8203;mhevery](https://togithub.com/mhevery) in [https://github.com/BuilderIO/qwik/pull/5187](https://togithub.com/BuilderIO/qwik/pull/5187)
-   chore: update Prisma versions package.json by [@&#8203;ruheni](https://togithub.com/ruheni) in [https://github.com/BuilderIO/qwik/pull/5184](https://togithub.com/BuilderIO/qwik/pull/5184)
-   docs: fix routing page - add missing attributes. by [@&#8203;hamatoyogi](https://togithub.com/hamatoyogi) in [https://github.com/BuilderIO/qwik/pull/5188](https://togithub.com/BuilderIO/qwik/pull/5188)
-   fix(qwik-city): do not write into a destroyed stream by [@&#8203;tuurbo](https://togithub.com/tuurbo) in [https://github.com/BuilderIO/qwik/pull/5172](https://togithub.com/BuilderIO/qwik/pull/5172)
-   fix(qwik-city): wait until action fully complete before resolving its… by [@&#8203;mhevery](https://togithub.com/mhevery) in [https://github.com/BuilderIO/qwik/pull/5190](https://togithub.com/BuilderIO/qwik/pull/5190)

##### New Contributors

-   [@&#8203;ilianiv](https://togithub.com/ilianiv) made their first contribution in [https://github.com/BuilderIO/qwik/pull/5159](https://togithub.com/BuilderIO/qwik/pull/5159)
-   [@&#8203;pipisso](https://togithub.com/pipisso) made their first contribution in [https://github.com/BuilderIO/qwik/pull/5169](https://togithub.com/BuilderIO/qwik/pull/5169)
-   [@&#8203;sapphi-red](https://togithub.com/sapphi-red) made their first contribution in [https://github.com/BuilderIO/qwik/pull/5161](https://togithub.com/BuilderIO/qwik/pull/5161)
-   [@&#8203;Aslemammad](https://togithub.com/Aslemammad) made their first contribution in [https://github.com/BuilderIO/qwik/pull/5180](https://togithub.com/BuilderIO/qwik/pull/5180)

**Full Changelog**: QwikDev/qwik@v1.2.11...v1.2.12

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 9pm on sunday" (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/ascorbic/unpic-img).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi45Ny4xIiwidXBkYXRlZEluVmVyIjoiMzYuOTcuMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->
@chuikoffru
Copy link

Hello, @EamonHeffernan

Thanks a lot for this adapter!

I faced with trouble of inner pages in Bun 1.0.4.

Step by step:

  1. bun create qwik@latest, select Demo Site with demo pages
  2. bun run qwik add bun
  3. bun run build
  4. bun run serve
  5. Open localhost:3000, and main page working well
  6. Go to http://localhost:3000/demo/flower - it's not ok. It's not working.

Could you fix please. Thanks again.

@EamonHeffernan
Copy link
Contributor Author

Hello, @EamonHeffernan

Thanks a lot for this adapter!

I faced with trouble of inner pages in Bun 1.0.4.

Step by step:

1. `bun create qwik@latest`, select Demo Site with demo pages

2. `bun run qwik add bun`

3. `bun run build`

4. `bun run serve`

5. Open localhost:3000, and main page working well

6. Go to http://localhost:3000/demo/flower - it's not ok. It's not working.

Could you fix please. Thanks again.

Hi,
Very sorry that that got overlooked, I've tracked down the issues with that.
I've opened the pull request #5272 that resolves this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP: starters STATUS-2: PR waiting for review This PR is waiting for review and approval before merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[✨]Integrate Qwik with Bun
6 participants