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

Patch #11858

Merged
merged 55 commits into from
Jun 18, 2024
Merged

Patch #11858

merged 55 commits into from
Jun 18, 2024

Conversation

zackradisic
Copy link
Contributor

@zackradisic zackradisic commented Jun 13, 2024

What does this PR do?

Fixes #11719

  • added docs
  • added patch/patch-commit in help
  • added bun patch --commit
  • fix various bugs/issues with patchedDependencies
    • have to calculate patchfile hashes everytime because they can be changed
    • fix infinite loop (stuck on Resolving...)
    • fix package not being updated when patch is removed
    • make it work when a package is already patched
    • fix a bunch of things with patchfile postprocessing
  • make patching work directly inside of node_modules, no need for temp dir

@zackradisic zackradisic marked this pull request as ready for review June 13, 2024 23:49
Copy link
Contributor

github-actions bot commented Jun 14, 2024

@zackradisic, your commit has failing tests :(

💻 1 failing tests Darwin x64 baseline

  • test/integration/expo-app/expo.test.ts 1 failing

🐧💪 1 failing tests Linux AARCH64

  • test/js/bun/shell/leak.test.ts 2 failing

🪟💻 1 failing tests Windows x64 baseline

  • test/cli/install/migration/out-of-sync.test.ts 1 failing

🪟💻 1 failing tests Windows x64

  • test/js/bun/io/bun-write.test.js 1 failing

View logs

@Jarred-Sumner
Copy link
Collaborator

image

src/cli.zig Outdated Show resolved Hide resolved
const pkg = lockfile.packages.get(pkg_id);
if (version) |v| {
const label = std.fmt.bufPrint(buf[0..], "{}", .{pkg.resolution.fmt(strbuf, .posix)}) catch @panic("Resolution name too long");
if (std.mem.eql(u8, label, v)) break :brk .{ @intCast(dep_id), pkg_id };
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels like not a great way to do it but I'm not sure

@dylan-conway wdyt

Copy link
Member

Choose a reason for hiding this comment

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

hmm yeah this won't work if version is a range like ^4.0.0 because the resolution will have the exact version. I'm not sure if version ranges are allowed with pnpm patch so it might be fine

.{name},
);
var i: usize = 0;
const pkg_hashes = lockfile.packages.items(.name_hash);
Copy link
Member

Choose a reason for hiding this comment

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

in the future, this is where you would get the package_id for the current workspace and only loop through it's packages, instead of all packages

@Jarred-Sumner Jarred-Sumner merged commit 7c27f3f into main Jun 18, 2024
48 of 53 checks passed
@Jarred-Sumner Jarred-Sumner deleted the zack/patch-fixes branch June 18, 2024 23:34
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.

Fix bugs and DX issues with bun patch
3 participants