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

fix(use): create package.json when calling corepack use on empty dir #350

Merged
merged 8 commits into from
Jan 12, 2024

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 6, 2024

Fixes: #347

Comment on lines 103 to 111
try {
file = await fs.promises.open(manifestPath, `r`);
} catch (err) {
if ((err as NodeError)?.code === `ENOENT`) continue;
throw err;
}

const content = await fs.promises.readFile(manifestPath, `utf8`);
const content = await file.readFile(`utf8`);
await file.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just use readFile and assign {} to data wen catching ENOENT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, the only anti-pattern is to rely on existsSync, as its result might be outdated by the time the readFile calls is processed.

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

Could you add a test that makes sure the parent package.json is updated when ran from a sub-folder?

corepack use foo@1
mkdir src
cd src
corepack use foo@2

Copy link
Contributor

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

Couple of nits, but LGTM overall

sources/commands/Use.ts Outdated Show resolved Hide resolved
sources/specUtils.ts Show resolved Hide resolved
sources/specUtils.ts Outdated Show resolved Hide resolved
Co-authored-by: Maël Nison <nison.mael@gmail.com>
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

LGTM.

NIT: This seems more like a new feature (feat:) than a bugfix (fix:).

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 12, 2024

NIT: This seems more like a new feature (feat:) than a bugfix (fix:).

I chose fix because it was already the intent of the code, as you can see the 'NoProject' string was already supposed to mark the creation of a package.json. No strong feelings though

@merceyz
Copy link
Member

merceyz commented Jan 12, 2024

Right, in that case I agree, it's a bugfix 👍

@aduh95 aduh95 merged commit 2950a8a into main Jan 12, 2024
9 of 10 checks passed
@aduh95 aduh95 deleted the fix-use-no-pk-json branch January 12, 2024 17:52
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.

corepack use fails with ENOENT: no such file or directory, open '/app/package.json'
3 participants