Skip to content

Commit

Permalink
fix(core): Ignore empty bin value in Manifest#load (#5743)
Browse files Browse the repository at this point in the history
**What's the problem this PR addresses?**

Some packages pass an invalid empty string for a `bin` entry in their
`package.json`, for example
[[email protected]](https://github.com/webpack-contrib/mini-css-extract-plugin/blob/v0.4.5/package.json#L10)

The NodeModules linker already ignores empty entries:

https://github.com/yarnpkg/berry/blob/master/packages/plugin-nm/sources/NodeModulesLinker.ts#L982

However `scriptUtils` was not, and would append `""` to the bin path
(resulting in the directory itself, not a binary)

Resolves #5669 

**How did you fix it?**

Instead of trying to fix this everywhere that might use a manifest
`.bin` property, I just remove these entries from the manifest when it
is being loaded, since an empty `bin` does not seem to be a valid entry.
The npm docs for `package.json` make no mention of any special case for
when `bin` is set to an empty string.

**Checklist**
<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [X] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [X] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [X] I will check that all automated PR checks pass before the PR gets
reviewed.
  • Loading branch information
rally25rs authored Sep 22, 2023
1 parent dc72cba commit 4ccec80
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 2 deletions.
34 changes: 34 additions & 0 deletions .yarn/versions/fe7910e2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
releases:
"@yarnpkg/cli": patch
"@yarnpkg/core": patch

declined:
- "@yarnpkg/plugin-compat"
- "@yarnpkg/plugin-constraints"
- "@yarnpkg/plugin-dlx"
- "@yarnpkg/plugin-essentials"
- "@yarnpkg/plugin-exec"
- "@yarnpkg/plugin-file"
- "@yarnpkg/plugin-git"
- "@yarnpkg/plugin-github"
- "@yarnpkg/plugin-http"
- "@yarnpkg/plugin-init"
- "@yarnpkg/plugin-interactive-tools"
- "@yarnpkg/plugin-link"
- "@yarnpkg/plugin-nm"
- "@yarnpkg/plugin-npm"
- "@yarnpkg/plugin-npm-cli"
- "@yarnpkg/plugin-pack"
- "@yarnpkg/plugin-patch"
- "@yarnpkg/plugin-pnp"
- "@yarnpkg/plugin-pnpm"
- "@yarnpkg/plugin-stage"
- "@yarnpkg/plugin-typescript"
- "@yarnpkg/plugin-version"
- "@yarnpkg/plugin-workspace-tools"
- "@yarnpkg/builder"
- "@yarnpkg/doctor"
- "@yarnpkg/extensions"
- "@yarnpkg/nm"
- "@yarnpkg/pnpify"
- "@yarnpkg/sdks"
6 changes: 4 additions & 2 deletions packages/yarnpkg-core/sources/Manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -278,14 +278,16 @@ export class Manifest {

this.bin = new Map();
if (typeof data.bin === `string`) {
if (this.name !== null) {
if (data.bin.trim() === ``) {
errors.push(new Error(`Invalid bin field`));
} else if (this.name !== null) {
this.bin.set(this.name.name, normalizeSlashes(data.bin));
} else {
errors.push(new Error(`String bin field, but no attached package name`));
}
} else if (typeof data.bin === `object` && data.bin !== null) {
for (const [key, value] of Object.entries(data.bin)) {
if (typeof value !== `string`) {
if (typeof value !== `string` || value.trim() === ``) {
errors.push(new Error(`Invalid bin definition for '${key}'`));
continue;
}
Expand Down
20 changes: 20 additions & 0 deletions packages/yarnpkg-core/tests/Manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,25 @@ describe(`Manifest`, () => {
[`start`, `node index.js`],
]);
});

it(`should preserve bin if a string was specified`, () => {
const manifest = Manifest.fromText(`{ "name": "name", "bin": "./bin.js" }`);
expect(manifest.exportTo({}).bin).toEqual(`./bin.js`);
});

it(`should preserve bin map if a hash was specified`, () => {
const manifest = Manifest.fromText(`{ "name": "name", "bin": { "bin1": "./bin1.js", "bin2": "./bin2.js" } }`);
expect(manifest.exportTo({}).bin).toEqual({bin1: `./bin1.js`, bin2: `./bin2.js`});
});

it(`should remove bin if an empty path was specified`, () => {
const manifest = Manifest.fromText(`{ "name": "name", "bin": "" }`);
expect(manifest.exportTo({}).bin).toEqual(undefined);
});

it(`should remove entries from bin map if an empty path was specified`, () => {
const manifest = Manifest.fromText(`{ "name": "name", "bin": { "bin1": " ", "bin2": "./bin2.js" } }`);
expect(manifest.exportTo({}).bin).toEqual({bin2: `./bin2.js`});
});
});
});

0 comments on commit 4ccec80

Please sign in to comment.