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

[bun] Add support for text-based lockfile format #33825

Merged
merged 7 commits into from
Jan 6, 2025

Conversation

tharakadesilva
Copy link
Contributor

@tharakadesilva tharakadesilva commented Dec 24, 2024

Why

Bun v1.1.39 introduced a new text-based lockfile format (bun.lock) as an alternative to the binary format (bun.lockb). This change adds support for detecting and using both formats.

https://bun.sh/blog/bun-lock-text-lockfile

Similar to: expo/eas-cli#2782 (I am not sure how EAS resolves the package manager, hopefully through Expo?)

How

The changes were implemented by:

  1. Adding support for both lockfile formats in the package manager resolution logic:

    • Added BUN_TEXT_LOCK_FILE constant for the new 'bun.lock' format
    • Updated resolvePackageManager to check for both lockfile formats
    • Modified the lockFiles record to support multiple lockfile paths per package manager
  2. Updated documentation:

    • Added new section about text-based lockfile in using-bun.mdx
    • Updated CLI help text to reflect support for both formats
    • Updated EAS build documentation to mention both lockfile formats
  3. Added comprehensive test coverage:

    • Tests for text lockfile only
    • Tests for binary lockfile only
    • Tests for both lockfiles present
    • Tests for monorepo workspace scenarios
  4. Updated cleanup utilities to handle both lockfile formats:

    • Modified reinstallPackagesAsync to clean up both bun.lock and bun.lockb
    • Updated gitignore to include both lockfile formats

Test Plan

  1. Basic functionality test:
expo/expo/packages/@expo/cli/build/bin/cli install @supabase/supabase-js
  • Without changes: Incorrectly picks up npm as the package manager
  • With changes: Correctly identifies bun as the package manager when either bun.lock or bun.lockb is present
  1. Automated tests:
  • Added comprehensive test suite in nodeManagers-test.ts covering:
    • Package manager resolution with text lockfile
    • Package manager resolution with binary lockfile
    • Package manager resolution with both lockfiles
    • Monorepo workspace scenarios
    • Various lockfile combinations

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Dec 24, 2024
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Dec 24, 2024
@tharakadesilva tharakadesilva force-pushed the main branch 3 times, most recently from 6e0558a to a1118f9 Compare December 24, 2024 01:20
@tharakadesilva tharakadesilva marked this pull request as ready for review December 24, 2024 01:20
Copy link
Contributor

github-actions bot commented Dec 24, 2024

Subscribed to pull request

File Patterns Mentions
docs/** @Simek, @amandeepmittal
packages/@expo/cli/** @EvanBacon, @byCedric
packages/@expo/package-manager/** @EvanBacon, @byCedric
packages/create-expo-nightly/** @Kudo

Generated by CodeMention

@tharakadesilva tharakadesilva changed the title feat(bun): add support for text-based lockfile format [bun] Add support for text-based lockfile format Dec 24, 2024
@smaccoun
Copy link

Hope this gets merged soon. This creates a lot of unexpected error when building/running if you don't realize what's happening, especially if you are working in a monorepo with bun

@tharakadesilva
Copy link
Contributor Author

I have a feeling that this will only solve expo cli, not eas.

If we are not doing already, we should add a command to expo cli that we can then use in eas.

I'll try to check on this tomorrow.

@tharakadesilva
Copy link
Contributor Author

Someone from EAS might have better information than what I can find from the existing code base.. I will wait for someone to reply.

@Kudo
Copy link
Contributor

Kudo commented Jan 3, 2025

sorry that most people were off during the holiday season and would back from next week. i can check how eas support bun under the hood on the weekend.

thanks for having the great pr 👏

Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

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

please remove the unrelated using-bun.mdx and expo-cli.mdx file changes. also fix the lint error from package-manager.

🏃‍♀️ Running yarn clean
🏃‍♀️ Running yarn build
🏃‍♀️ Running yarn test --watch false --passWithNoTests --maxWorkers 1
🏃‍♀️ Running yarn lint --max-warnings 0
lint script failed, see process output:
stdout > yarn run v1.22.22
stdout > $ expo-module lint --max-warnings 0
stdout > 
stdout > /home/runner/work/expo/expo/packages/@expo/package-manager/src/utils/nodeManagers.ts
stdout >   49:45  warning  Replace `file` with `(file)`  prettier/prettier
stdout >   55:37  warning  Replace `file` with `(file)`  prettier/prettier
stdout > 
stdout > ✖ 2 problems (0 errors, 2 warnings)
stdout >   0 errors and 2 warnings potentially fixable with the `--fix` option.
stdout > 
stdout > info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
stderr > ESLint found too many warnings (maximum: 0).
stderr > error Command failed with exit code 1.

@Kudo
Copy link
Contributor

Kudo commented Jan 3, 2025

i can check how eas support bun under the hood on the weekend.

for what i can tell, eas will just use bun install to install dependencies. as long as bun.lock were there in the project with newer bun binary, it should be fine to run bun install. after this pr landed and @expo/package-manger can resolve the correct package manager and fix the expo/eas-cli#2782 issue. we don't need to do any other changes from eas side.

Bun v1.1.39 introduced a new text-based lockfile format (bun.lock) as an alternative to the binary format (bun.lockb). This change adds support for detecting and using both formats.

Changes:
- Add BUN_TEXT_LOCK_FILE constant for the new 'bun.lock' format
- Update package manager resolution to check for both lockfile formats
- Update documentation to explain the new text-based lockfile format and its benefits
- Add comprehensive test coverage for both lockfile formats

The text-based lockfile will become the default in Bun v1.2. Until then, users need to explicitly opt-in using the --save-text-lockfile flag.
@tharakadesilva
Copy link
Contributor Author

tharakadesilva commented Jan 5, 2025

please remove the unrelated using-bun.mdx

Removed the using-bun.mdx file as that is not related to this change and is in EAS.

we don't need to do any other changes from eas side.

If this is the case, should we still keep the doc change?

also fix the lint error from package-manager.

Done!

Thanks for the quick review and checking on things, Kudo!

Copy link
Member

@amandeepmittal amandeepmittal left a comment

Choose a reason for hiding this comment

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

The docs change lg! Thank you for updating Expo CLI reference to mention the new lock file extension.

Copy link
Member

@byCedric byCedric left a comment

Choose a reason for hiding this comment

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

Wow, I'm impressed. You not only added support for bun.lock, you also went the extra mile of updating the docs, --help output, and tests (fixtures). Thanks for that ❤️

Only left 2 suggestions for the changelogs, but these are optional.

@byCedric
Copy link
Member

byCedric commented Jan 6, 2025

I think the expo install --help snapshots also needs to be updated. You can do this locally through:

  • cd ./packages/@expo/cli
  • yarn test:e2e -u install-test

Or modify the snapshot manually at https://github.com/expo/expo/blob/main/packages/%40expo/cli/e2e/__tests__/install-test.ts#L50.

Let me know if you need help with that.


Edit: went ahead and updated the snapshot so we can merge it now 😄 Again, thanks!

@byCedric byCedric merged commit a96a6fe into expo:main Jan 6, 2025
2 checks passed
Kudo pushed a commit that referenced this pull request Jan 7, 2025
Bun v1.1.39 introduced a new text-based lockfile format (bun.lock) as an
alternative to the binary format (bun.lockb). This change adds support
for detecting and using both formats.

https://bun.sh/blog/bun-lock-text-lockfile

Similar to: expo/eas-cli#2782 (I am not sure
how EAS resolves the package manager, hopefully through Expo?)

The changes were implemented by:

1. Adding support for both lockfile formats in the package manager
resolution logic:
   - Added `BUN_TEXT_LOCK_FILE` constant for the new 'bun.lock' format
   - Updated `resolvePackageManager` to check for both lockfile formats
- Modified the lockFiles record to support multiple lockfile paths per
package manager

2. Updated documentation:
   - Added new section about text-based lockfile in using-bun.mdx
   - Updated CLI help text to reflect support for both formats
   - Updated EAS build documentation to mention both lockfile formats

3. Added comprehensive test coverage:
   - Tests for text lockfile only
   - Tests for binary lockfile only
   - Tests for both lockfiles present
   - Tests for monorepo workspace scenarios

4. Updated cleanup utilities to handle both lockfile formats:
- Modified `reinstallPackagesAsync` to clean up both `bun.lock` and
`bun.lockb`
   - Updated gitignore to include both lockfile formats

1. Basic functionality test:

```bash
expo/expo/packages/@expo/cli/build/bin/cli install @supabase/supabase-js
```

- Without changes: Incorrectly picks up npm as the package manager
- With changes: Correctly identifies bun as the package manager when
either `bun.lock` or `bun.lockb` is present

2. Automated tests:
- Added comprehensive test suite in `nodeManagers-test.ts` covering:
  - Package manager resolution with text lockfile
  - Package manager resolution with binary lockfile
  - Package manager resolution with both lockfiles
  - Monorepo workspace scenarios
  - Various lockfile combinations

<!--
Please check the appropriate items below if they apply to your diff.
-->

- [x] I added a `changelog.md` entry and rebuilt the package sources
according to [this short
guide](https://github.com/expo/expo/blob/main/CONTRIBUTING.md#-before-submitting)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).
- [x] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)

---------

Co-authored-by: Cedric van Putten <[email protected]>
(cherry picked from commit a96a6fe)
@Kudo Kudo added the published Changes from the PR have been published to npm label Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants