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

chore: avoid require to load package.json #1890

Merged
merged 1 commit into from Nov 12, 2023

Conversation

Uzlopak
Copy link
Collaborator

@Uzlopak Uzlopak commented Nov 11, 2023

We avoid with this PR to load some arbitrary file via require.
Require could result in issues in esm environments.

@Uzlopak Uzlopak requested a review from a team as a code owner November 11, 2023 01:50
@Uzlopak Uzlopak force-pushed the improve-cov-manifest-creation branch 2 times, most recently from 12957cc to 81cddd5 Compare November 11, 2023 02:07
@Uzlopak Uzlopak force-pushed the improve-cov-manifest-creation branch 2 times, most recently from f5c8fe5 to d2608c1 Compare November 11, 2023 17:53
@Uzlopak Uzlopak force-pushed the improve-cov-manifest-creation branch from d2608c1 to 71e7d12 Compare November 11, 2023 23:44
@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Nov 11, 2023

@AaronDewes
@gr2m

PTAL

The new function has 100 % test coverage and is implemented as resilient as possible.

): { [key: string]: any } {
let pkgContent;
try {
pkgContent = fs.readFileSync(filepath, "utf8");
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that we have sync FS access, but that's the case today as well, we can address that in a future change, it's internal anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As long as we use express we will have issues because express can not have async handlers.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, good to know.

@gr2m gr2m merged commit 3694baf into probot:beta Nov 12, 2023
8 checks passed
Copy link

🎉 This PR is included in version 13.0.0-beta.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants