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 support with npm fallback #29267

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

stephenjason89
Copy link

@stephenjason89 stephenjason89 commented Oct 2, 2024

Closes #28164

What I did

Added bun support and npm fallback to those where bun lacks

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Greptile Summary

This PR introduces support for the Bun package manager in Storybook, addressing issue #28164.

  • Added new BUNProxy class in code/core/src/common/js-package-manager/BUNProxy.ts for Bun-specific operations
  • Updated PackageManagerName type in JsPackageManager to include 'bun'
  • Implemented Bun package management methods with npm fallbacks for unsupported operations
  • Follows similar structure to existing proxy classes for PNPM, NPM, and Yarn
  • Enables Storybook installation, upgrade, automigration, and execution with Bun as the package manager

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings

EUNKNOWNTYPE: 'Unknown type encountered.',
};

export class BUNProxy extends JsPackageManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider renaming to BunProxy for consistency with other package manager proxies

Comment on lines +84 to +86
async getNpmVersion(): Promise<string> {
return this.executeCommand({ command: 'npm', args: ['--version'] });
}
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: getNpmVersion uses npm instead of bun. Consider implementing a getBunVersion method

});
}

public async findInstallations(pattern: string[], { depth = 99 }: { depth?: number } = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: findInstallations method uses npm instead of bun. Implement a bun-specific version or handle potential differences

Comment on lines +186 to +195
public async getRegistryURL() {
const res = await this.executeCommand({
command: 'npm',
// "npm config" commands are not allowed in workspaces per default
// https://github.com/npm/cli/issues/6099#issuecomment-1847584792
args: ['config', 'get', 'registry', '-ws=false', '-iwr'],
});
const url = res.trim();
return url === 'undefined' ? undefined : url;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: getRegistryURL uses npm. Implement a bun-specific method or handle potential differences

});
}

protected async runGetVersions<T extends boolean>(
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: runGetVersions uses npm. Implement a bun-specific method or handle potential differences

Comment on lines +309 to +310
infoCommand: 'npm ls --depth=1',
dedupeCommand: 'npm dedupe',
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: infoCommand and dedupeCommand use npm. Update to use bun equivalents if available

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.

[Bug]: Support bun as a package manager
1 participant