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

feat: allow checkPlatform to override execution environment #65

Merged
merged 5 commits into from
Aug 7, 2023
+38 −4
Merged

feat: allow checkPlatform to override execution environment #65

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7530e75
allow checkPlatform to override the execution environment
yukukotani Aug 4, 2023
ff74213
remove needless comment
yukukotani Aug 5, 2023
d9b4234
add test
yukukotani Aug 7, 2023
76b0df6
remove needless typeguard
yukukotani Aug 7, 2023
c5734ed
Update test/check-platform.js
yukukotani Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ npm is unable to install the package properly for some reason.

Error code: 'EBADENGINE'

### .checkPlatform(pkg, force)
### .checkPlatform(pkg, force, environment)

Check if a package's `os`, `cpu` and `libc` match the running system.

`force` argument skips all checks.

`environment` overrides the execution environment which comes from `process.platform` and `process.arch` by default. `environment.os` and `environment.cpu` are available.

Error code: 'EBADPLATFORM'
6 changes: 3 additions & 3 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ const checkEngine = (target, npmVer, nodeVer, force = false) => {

const isMusl = (file) => file.includes('libc.musl-') || file.includes('ld-musl-')

const checkPlatform = (target, force = false) => {
const checkPlatform = (target, force = false, environment = {}) => {
if (force) {
return
}

const platform = process.platform
const arch = process.arch
const platform = environment && environment.os ? environment.os : process.platform
Copy link
Member

Choose a reason for hiding this comment

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

Since we're defaulting environment to {} here we don't need the environment && guard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wraithgar Callers of this function may still pass null or undefined for this argument while the default value is {}. How about leaving the environment && guard as it is and changing the default value to undefined?

Copy link
Member

Choose a reason for hiding this comment

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

We typically don't guard against bad input. It (arguably unnecessarily) increases the cognitive load of maintaining this module. This is a style choice inside of the npm codebase, we don't need to support the extra falsey value, folks consuming this can omit the parameter.

Copy link
Contributor Author

@yukukotani yukukotani Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@wraithgar Okay, that makes sense! Let's prioritize the consistency across the codebase. 76b0df6

const arch = environment && environment.cpu ? environment.cpu : process.arch
const osOk = target.os ? checkList(platform, target.os) : true
const cpuOk = target.cpu ? checkList(arch, target.cpu) : true

Expand Down