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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Jan 12, 2024
Merged

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

merged 8 commits into from
Jan 12, 2024

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 6, 2024

Fixes: #347

arcanis
arcanis reviewed Jan 8, 2024
View reviewed changes
sources/specUtils.ts Outdated
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.

merceyz
merceyz reviewed Jan 10, 2024
View reviewed changes
Copy link
Member

@merceyz merceyz left a comment •
edited
Loading

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

arcanis
arcanis approved these changes Jan 11, 2024
View reviewed changes
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
aduh95
aduh95 commented Jan 11, 2024
View reviewed changes
sources/specUtils.ts Outdated Show resolved Hide resolved
merceyz
merceyz approved these changes Jan 12, 2024
View reviewed changes
Copy link
Member

@merceyz merceyz left a comment •
edited
Loading

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
@github-actions github-actions bot mentioned this pull request Jan 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merceyz merceyz merceyz approved these changes

@arcanis arcanis arcanis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
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
@aduh95 @merceyz @arcanis