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 install does not use specified version of a dependency even when it satisfies the sub-dependency requirements #6245

Closed
ricklove opened this issue Oct 2, 2023 · 17 comments
Labels
bug Something isn't working bun install Something that relates to the npm-compatible client

Comments

@ricklove
Copy link

ricklove commented Oct 2, 2023

What version of Bun is running?

1.0.3+25e69c71e70ac8a0a88f9cf15b4057bd7b2a633a

What platform is your computer?

Linux 5.15.90.1-microsoft-standard-WSL2 x86_64 x86_64

What steps can reproduce the bug?

In my case, I was trying to use expo with expo router and nativewind. postcss introduced a breaking change in a minor version, so I needed to use specific versions of each:

Minimal package.json:

{
  "name": "bun-bugs",
  "module": "index.ts",
  "type": "module",
  "devDependencies": {
    "bun-types": "latest"
  },
  "peerDependencies": {
    "typescript": "5.0.0"
  },
  "dependencies": {
    "expo": "49.0.13",
    "postcss": "8.4.21"
  }
}

What is the expected behavior?

Only version "8.4.21" should be used:

[email protected], postcss@~8.4.21:
  version "8.4.21"
  resolved "https://registry.npmjs.org/postcss/-/postcss-8.4.21.tgz"
  integrity sha512-tP7u/Sn/dVxK2NnruI4H9BG+x+Wxz6oeZ1cJ8P6G/PZY0IKk4k/63TDsQf2kQq3+qoJeLm2kIBUNlZe3zgb4Zg==
  dependencies:
    nanoid "^3.3.4"
    picocolors "^1.0.0"
    source-map-js "^1.0.2"

What do you see instead?

It's not using the specified version to satisfy the tilda version:

[email protected]:
  version "8.4.21"
  resolved "https://registry.npmjs.org/postcss/-/postcss-8.4.21.tgz"
  integrity sha512-tP7u/Sn/dVxK2NnruI4H9BG+x+Wxz6oeZ1cJ8P6G/PZY0IKk4k/63TDsQf2kQq3+qoJeLm2kIBUNlZe3zgb4Zg==
  dependencies:
    nanoid "^3.3.4"
    picocolors "^1.0.0"
    source-map-js "^1.0.2"

postcss@~8.4.21:
  version "8.4.31"
  resolved "https://registry.npmjs.org/postcss/-/postcss-8.4.31.tgz"
  integrity sha512-PS08Iboia9mts/2ygV3eLpY5ghnUcfLV/EXTOW1E2qYxJKGGBUtNjN76FYHnMs36RmARn41bC0AZmn+rR0OVpQ==
  dependencies:
    nanoid "^3.3.6"
    picocolors "^1.0.0"
    source-map-js "^1.0.2"

Additional information

Minimal reproduction repo: https://github.com/ricklove/bun-bugs

@ricklove ricklove added the bug Something isn't working label Oct 2, 2023
@ricklove
Copy link
Author

ricklove commented Oct 2, 2023

Workaround: I had to use yarn for the expo app package and it installed dependencies in the local node_modules which are used for it's tooling

@Electroid Electroid added the bun install Something that relates to the npm-compatible client label Oct 2, 2023
@ricklove
Copy link
Author

ricklove commented Oct 2, 2023

Notes - looking for bug:

The order of semver seems correct, a specific tag should be greater than any prefix. However, the name orderWithoutTag is incorrect since this version does consider the tag also (though this does not imply a bug, but checking how this is used next):

https://github.com/oven-sh/bun/blob/a6af1c89b954c56493d457d33b7f066e1f195afa/src/install/semver.zig#L721C1-L739C6

    pub fn orderWithoutTag(
        lhs: Version,
        rhs: Version,
    ) std.math.Order {
        if (lhs.major < rhs.major) return .lt;
        if (lhs.major > rhs.major) return .gt;
        if (lhs.minor < rhs.minor) return .lt;
        if (lhs.minor > rhs.minor) return .gt;
        if (lhs.patch < rhs.patch) return .lt;
        if (lhs.patch > rhs.patch) return .gt;

        if (lhs.tag.hasPre()) {
            if (!rhs.tag.hasPre()) return .lt;
        } else {
            if (rhs.tag.hasPre()) return .gt;
        }

        return .eq;
    }

Update: Actually, not sure if pre refers to the tag prefix or prerelease, but pre-release is really a part of patch, but maybe it was moved to the tag?

Update 2: It does appear that hasPre is dealing with prerelease version, so it is correct for any prerelease to be before the version without prerelease: https://semver.org/#spec-item-9

@ricklove
Copy link
Author

ricklove commented Oct 2, 2023

It does not appear that VersionedURL contains any data about the ~ or ^ markers for resolution.

Update: This is part of Query.token.tag

'~' => {

@ricklove
Copy link
Author

ricklove commented Oct 2, 2023

https://github.com/oven-sh/bun/blob/a6af1c89b954c56493d457d33b7f066e1f195afa/src/install/install.zig#L2422C1-L2423C74

In resolveFromDiskCache it appears to use the first item that matches after being sorted by Semver.Version.sortGt - this seems correct.

@Jarred-Sumner
Copy link
Collaborator

Note that resolveFromDiskCache is only used for auto-installs at runtime (not for bun install)

@ricklove
Copy link
Author

ricklove commented Oct 2, 2023

I tried disabling global cache and removing the higher versions and that didn't change anything, so doesn't seem to be the source of the issue.

@ricklove
Copy link
Author

ricklove commented Oct 3, 2023

Need to add a test: expect.range("~1.2.1", "1.2.4", @src()); since the existing tests only check ~ in the case of a missing or same patch version right now. No tests check that ~ is in range for the same minor version but different patch version.

test "Range parsing" {
    defer expect.done(@src());

    expect.range("~1.2.1", "1.2.4", @src()); // Need to add
    expect.range("~1.2.3", "1.2.3", @src());
    expect.range("~1.2", "1.2.0", @src());

https://github.com/oven-sh/bun/blob/a6af1c89b954c56493d457d33b7f066e1f195afa/src/install/semver.zig#L2158C45-L2158C45

I don't have this repo building on my system yet, so can't compile to check if this passes.

The code appears correct, but need to add the test case anyway:

https://github.com/oven-sh/bun/blob/a6af1c89b954c56493d457d33b7f066e1f195afa/src/install/semver.zig#L1551C28-L1551C28

@ricklove
Copy link
Author

ricklove commented Oct 3, 2023

@ricklove
Copy link
Author

ricklove commented Oct 3, 2023

Reproduction in this PR on my repo:

ricklove/ricklove-universal-app#1

@ricklove
Copy link
Author

ricklove commented Oct 3, 2023

Ok, I added a minimal reproduction here: https://github.com/ricklove/bun-bugs

8.4.21 should satisfy postcss@~8.4.21 so why is it using version "8.4.31"

[email protected]:
  version "8.4.21"
  resolved "https://registry.npmjs.org/postcss/-/postcss-8.4.21.tgz"
  integrity sha512-tP7u/Sn/dVxK2NnruI4H9BG+x+Wxz6oeZ1cJ8P6G/PZY0IKk4k/63TDsQf2kQq3+qoJeLm2kIBUNlZe3zgb4Zg==
  dependencies:
    nanoid "^3.3.4"
    picocolors "^1.0.0"
    source-map-js "^1.0.2"

postcss@~8.4.21:
  version "8.4.31"
  resolved "https://registry.npmjs.org/postcss/-/postcss-8.4.31.tgz"
  integrity sha512-PS08Iboia9mts/2ygV3eLpY5ghnUcfLV/EXTOW1E2qYxJKGGBUtNjN76FYHnMs36RmARn41bC0AZmn+rR0OVpQ==
  dependencies:
    nanoid "^3.3.6"
    picocolors "^1.0.0"
    source-map-js "^1.0.2"

@Jarred-Sumner
Copy link
Collaborator

8.4.31 is valid for the selector postcss@~8.4.21. npm-compatible package managers must choose latest first in most cases

image

https://semver.npmjs.com/

@Jarred-Sumner
Copy link
Collaborator

What you really want is #1134

@ricklove
Copy link
Author

ricklove commented Oct 3, 2023

@Jarred-Sumner in any other package manager, if you specify a version that satisfies the requirements, that will have priority when resolving sub dependencies (within the same workspace)

@ricklove
Copy link
Author

ricklove commented Oct 3, 2023

Let me add npm, yarn, and pnpm results for comparison

@ricklove
Copy link
Author

ricklove commented Oct 3, 2023

@Jarred-Sumner Looks like I am wrong. Looks like yarn is indeterministic (in the original repro, yarn had different behavior than here):

[email protected]:
  version "8.4.21"
  resolved "https://registry.yarnpkg.com/postcss/-/postcss-8.4.21.tgz#c639b719a57efc3187b13a1d765675485f4134f4"
  integrity sha512-tP7u/Sn/dVxK2NnruI4H9BG+x+Wxz6oeZ1cJ8P6G/PZY0IKk4k/63TDsQf2kQq3+qoJeLm2kIBUNlZe3zgb4Zg==
  dependencies:
    nanoid "^3.3.4"
    picocolors "^1.0.0"
    source-map-js "^1.0.2"

postcss@~8.4.21:
  version "8.4.31"
  resolved "https://registry.yarnpkg.com/postcss/-/postcss-8.4.31.tgz#92b451050a9f914da6755af352bdc0192508656d"
  integrity sha512-PS08Iboia9mts/2ygV3eLpY5ghnUcfLV/EXTOW1E2qYxJKGGBUtNjN76FYHnMs36RmARn41bC0AZmn+rR0OVpQ==
  dependencies:
    nanoid "^3.3.6"
    picocolors "^1.0.0"
    source-map-js "^1.0.2"

@ricklove
Copy link
Author

ricklove commented Oct 3, 2023

Ok, I agree that #1134 would resolve this since my assumption does not seem to be in the specs for node module resolution.

Also, using overrides is a much more clear indication of intention for a case like this (where a breaking change was introduced in a dependency)

@Jarred-Sumner
Copy link
Collaborator

overrides is the intended solution for this and it has since been implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working bun install Something that relates to the npm-compatible client
Projects
None yet
Development

No branches or pull requests

3 participants