Skip to content

fix: bun detect #3791

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jul 25, 2024
Merged

fix: bun detect #3791

merged 6 commits into from
Jul 25, 2024

Conversation

keyding
Copy link
Contributor

@keyding keyding commented Jul 23, 2024 •
edited by pr-codex bot
Loading

Problem solved

Fix bun detection

Changes made

  • Internal API changes: packages/thirdweb/src/cli/bin.ts

How to test

bun packages/thirdweb/src/cli/bin.ts
bunx thirdweb create

Contributor NFT

Paste in your wallet address below and we will airdrop you a special NFT when your pull request is merged.

Address: 0xC2745cd26F703C869a6d4797472f64a599C4ED65

PR-Codex overview

This PR focuses on fixing bun detection in the bin.ts file of the thirdweb package.

Detailed summary

  • Updated the runner variable assignment from "bun" to "bunx" for bun detection.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jul 23, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: f77b221

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
thirdweb Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel Vercel
Copy link

vercel bot commented Jul 23, 2024

@keyding is attempting to deploy a commit to the thirdweb Team on Vercel.

A member of the Team first needs to authorize it.

@graphite-app Graphite App
Copy link
Contributor

graphite-app bot commented Jul 23, 2024

Your org requires the Graphite merge queue for merging into main

Add the label “merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added the TS SDK Involves changes to the v5 TypeScript SDK. label Jul 23, 2024
jnsdls
jnsdls reviewed Jul 23, 2024
View reviewed changes
Copy link
Member

@jnsdls jnsdls left a comment

Choose a reason for hiding this comment

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

thanks @keyding, mainly just one quick question

packages/thirdweb/src/cli/bin.ts
@@ -29,7 +29,7 @@ async function main() {
const isBunAvailable = (() => {
try {
const res = execSync("bun --version", {
stdio: "ignore",
// stdio: "ignore",
Copy link
Member

Choose a reason for hiding this comment

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

is this part of the fix or just left over?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of the fix, when stdio is set to ignore, res is null.

packages/thirdweb/src/cli/bin.ts
@@ -43,7 +43,7 @@ async function main() {

switch (true) {
case isBunAvailable:
runner = "bun";
runner = "bunx";
Copy link
Member

Choose a reason for hiding this comment

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

I think this makes sense

@vercel Vercel
Copy link

vercel bot commented Jul 23, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
thirdweb-www ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 25, 2024 1:46am

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Jul 23, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #3791 will improve performances by 60.57%

Comparing keyding:fix/thirdweb-bun (f77b221) with main (cb616f7)

Summary

⚡ 2 improvements
✅ 7 untouched benchmarks

Benchmarks breakdown

Benchmark main keyding:fix/thirdweb-bun Change
⚡ encode tx (contract abi) 530.1 µs 330.1 µs +60.57%
⚡ encode tx (prepared method) 475.7 µs 318.4 µs +49.38%

jnsdls
jnsdls approved these changes Jul 23, 2024
View reviewed changes
Copy link
Member

@jnsdls jnsdls left a comment

Choose a reason for hiding this comment

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

LGTM

@joaquim-verges joaquim-verges enabled auto-merge (squash) July 24, 2024 07:43
@jnsdls jnsdls disabled auto-merge July 24, 2024 18:01
@jnsdls jnsdls added the merge-queue Adds the pull request to Graphite's merge queue. label Jul 24, 2024
@graphite-app Graphite App
Copy link
Contributor

graphite-app bot commented Jul 24, 2024

Merge activity

  • Jul 24, 2:03 PM EDT: The merge label 'merge-queue' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.

@jnsdls jnsdls merged commit 9cc9f05 into thirdweb-dev:main Jul 25, 2024
24 of 27 checks passed
@jnsdls jnsdls mentioned this pull request Jul 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnsdls jnsdls jnsdls approved these changes

Assignees
No one assigned
Labels
merge-queue Adds the pull request to Graphite's merge queue. TS SDK Involves changes to the v5 TypeScript SDK.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@keyding @jnsdls @joaquim-verges