-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
module: expose getPackageJSON
utility
#55229
Changes from 33 commits
48ab696
dcddd1e
98c72f4
d4ede54
92f2c62
b83707b
e0beefa
6e715ec
458dcca
903d201
b567e9a
ed4db64
9a4b1d1
c23c8f1
c865d55
a9bf393
190e315
a9e3ded
926b2b0
485b1e3
f824ce9
7deeb7f
cf49f71
358486c
d9bdf23
bf5265d
3e1cda8
da29815
b531497
eef2b60
395cfa6
841848f
945c207
4adac4c
0064379
eed739c
864b98a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -217,6 +217,48 @@ added: v22.8.0 | |||||||||||||
* Returns: {string|undefined} Path to the [module compile cache][] directory if it is enabled, | ||||||||||||||
or `undefined` otherwise. | ||||||||||||||
|
||||||||||||||
### `module.getPackageJSON(startLocation[, everything])` | ||||||||||||||
|
||||||||||||||
<!-- YAML | ||||||||||||||
added: REPLACEME | ||||||||||||||
--> | ||||||||||||||
|
||||||||||||||
> Stability: 1.1 - Active Development | ||||||||||||||
|
||||||||||||||
* `startLocation` {string} A fully resolved FS path or file URL to start looking | ||||||||||||||
* `everything` {boolean} Whether to return the full contents of the found package.json | ||||||||||||||
* Returns: {Object | undefined} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Ditto to the rest There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the way the docs are formatted. There is no space between the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it actually matter or is it a style/consistency issue? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the linter will throw an error, but it's documented in the Style Guide (https://github.com/nodejs/node/blob/main/doc/README.md#function-arguments-and-returns) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah let's keep them consistent, i.e. without spaces |
||||||||||||||
* data: {Object} | ||||||||||||||
* name: {string | undefined} | ||||||||||||||
* type: {string | undefined} | ||||||||||||||
Comment on lines
+231
to
+233
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto with the rest
Suggested change
|
||||||||||||||
* exports: string | string\[] | Record\<string, unknown> | undefined | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a valid type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dunno if the validator considers it one, but it absolutely valid 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that we are not using TS nor JSDoc syntax, so it doesn't really make sense to claim it's "valid syntax" if the validator disagrees
Suggested change
|
||||||||||||||
* imports: string | string\[] | Record\<string, unknown> | undefined | ||||||||||||||
* … | ||||||||||||||
* path: {string} | ||||||||||||||
|
||||||||||||||
Retreives the contents and location of the package.json closest to the supplied `startLocation`; | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
this behaves identically to node's own lookup and consumption of package.json for a given module. | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
|
||||||||||||||
```mjs | ||||||||||||||
import { getPackageJSON } from 'node:module'; | ||||||||||||||
|
||||||||||||||
const somePackage = getPackageJSON(import.meta.resolve('some-package'), true); | ||||||||||||||
|
||||||||||||||
somePackage?.path; // '/…/node_modules/some-package/package.json' | ||||||||||||||
somePackage?.data.name; // 'some-package-real-name' | ||||||||||||||
somePackage?.data.types; // './index.d.ts' | ||||||||||||||
|
||||||||||||||
const thisParentPackage = getPackageJSON(import.meta.resolve('..')); | ||||||||||||||
|
||||||||||||||
thisParentPackage?.path; // '../../package.json' | ||||||||||||||
thisParentPackage?.data.type; // 'module' | ||||||||||||||
|
||||||||||||||
const thisSubPackage = getPackageJSON(import.meta.url); | ||||||||||||||
|
||||||||||||||
thisSubPackage?.path; // './package.json' | ||||||||||||||
thisSubPackage?.data.type; // 'commonjs' | ||||||||||||||
``` | ||||||||||||||
|
||||||||||||||
### `module.isBuiltin(moduleName)` | ||||||||||||||
|
||||||||||||||
<!-- YAML | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,31 +2,48 @@ | |
|
||
const { | ||
ArrayIsArray, | ||
ArrayPrototypeJoin, | ||
JSONParse, | ||
ObjectDefineProperty, | ||
StringPrototypeLastIndexOf, | ||
StringPrototypeSlice, | ||
StringPrototypeEndsWith, | ||
} = primordials; | ||
const { | ||
codes: { | ||
ERR_INVALID_ARG_VALUE, | ||
}, | ||
} = require('internal/errors'); | ||
const modulesBinding = internalBinding('modules'); | ||
const { resolve, sep } = require('path'); | ||
const path = require('path'); | ||
const { kEmptyObject } = require('internal/util'); | ||
const { fileURLToPath, URL } = require('internal/url'); | ||
const { | ||
validateBoolean, | ||
validateString, | ||
} = require('internal/validators'); | ||
|
||
/** | ||
* @typedef {import('typings/internalBinding/modules').DeserializedPackageConfig} DeserializedPackageConfig | ||
* @typedef {import('typings/internalBinding/modules').FullPackageConfig} FullPackageConfig | ||
* @typedef {import('typings/internalBinding/modules').RecognizedPackageConfig} RecognizedPackageConfig | ||
* @typedef {import('typings/internalBinding/modules').SerializedPackageConfig} SerializedPackageConfig | ||
*/ | ||
|
||
/** | ||
* @param {string} path | ||
* @param {import('typings/internalBinding/modules').SerializedPackageConfig} contents | ||
* @returns {import('typings/internalBinding/modules').PackageConfig} | ||
* @param {SerializedPackageConfig} contents | ||
* @returns {DeserializedPackageConfig} | ||
*/ | ||
function deserializePackageJSON(path, contents) { | ||
if (contents === undefined) { | ||
return { | ||
__proto__: null, | ||
JakobJingleheimer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
exists: false, | ||
pjsonPath: path, | ||
type: 'none', // Ignore unknown types for forwards compatibility | ||
data: { | ||
__proto__: null, | ||
type: 'none', // Ignore unknown types for forwards compatibility | ||
}, | ||
path, | ||
}; | ||
} | ||
|
||
let pjsonPath = path; | ||
const { | ||
0: name, | ||
1: main, | ||
|
@@ -37,36 +54,39 @@ function deserializePackageJSON(path, contents) { | |
} = contents; | ||
|
||
// This is required to be used in getPackageScopeConfig. | ||
if (optionalFilePath) { | ||
pjsonPath = optionalFilePath; | ||
} | ||
|
||
// The imports and exports fields can be either undefined or a string. | ||
// - If it's a string, it's either plain string or a stringified JSON string. | ||
// - If it's a stringified JSON string, it starts with either '[' or '{'. | ||
const requiresJSONParse = (value) => (value !== undefined && (value[0] === '[' || value[0] === '{')); | ||
const pjsonPath = optionalFilePath ?? path; | ||
|
||
return { | ||
__proto__: null, | ||
JakobJingleheimer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
exists: true, | ||
pjsonPath, | ||
name, | ||
main, | ||
type, | ||
// This getters are used to lazily parse the imports and exports fields. | ||
get imports() { | ||
const value = requiresJSONParse(plainImports) ? JSONParse(plainImports) : plainImports; | ||
ObjectDefineProperty(this, 'imports', { __proto__: null, value }); | ||
return this.imports; | ||
}, | ||
get exports() { | ||
const value = requiresJSONParse(plainExports) ? JSONParse(plainExports) : plainExports; | ||
ObjectDefineProperty(this, 'exports', { __proto__: null, value }); | ||
return this.exports; | ||
data: { | ||
__proto__: null, | ||
...(name !== null && { name }), | ||
JakobJingleheimer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
...(main != null && { main }), | ||
...(type != null && { type }), | ||
JakobJingleheimer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
...(plainImports != null && { | ||
// This getters are used to lazily parse the imports and exports fields. | ||
get imports() { | ||
const value = requiresJSONParse(plainImports) ? JSONParse(plainImports) : plainImports; | ||
ObjectDefineProperty(this, 'imports', { __proto__: null, value }); | ||
return this.imports; | ||
}, | ||
}), | ||
...(plainExports != null && { | ||
get exports() { | ||
const value = requiresJSONParse(plainExports) ? JSONParse(plainExports) : plainExports; | ||
ObjectDefineProperty(this, 'exports', { __proto__: null, value }); | ||
return this.exports; | ||
}, | ||
}), | ||
}, | ||
path: pjsonPath, | ||
}; | ||
} | ||
|
||
// The imports and exports fields can be either undefined or a string. | ||
// - If it's a string, it's either plain string or a stringified JSON string. | ||
// - If it's a stringified JSON string, it starts with either '[' or '{'. | ||
const requiresJSONParse = (value) => (value !== undefined && (value[0] === '[' || value[0] === '{')); | ||
|
||
/** | ||
* Reads a package.json file and returns the parsed contents. | ||
* @param {string} jsonPath | ||
|
@@ -75,7 +95,7 @@ function deserializePackageJSON(path, contents) { | |
* specifier?: URL | string, | ||
* isESM?: boolean, | ||
* }} options | ||
* @returns {import('typings/internalBinding/modules').PackageConfig} | ||
* @returns {RecognizedPackageConfig} | ||
*/ | ||
function read(jsonPath, { base, specifier, isESM } = kEmptyObject) { | ||
// This function will be called by both CJS and ESM, so we need to make sure | ||
|
@@ -87,52 +107,93 @@ function read(jsonPath, { base, specifier, isESM } = kEmptyObject) { | |
specifier == null ? undefined : `${specifier}`, | ||
); | ||
|
||
return deserializePackageJSON(jsonPath, parsed); | ||
const result = deserializePackageJSON(jsonPath, parsed); | ||
|
||
return { | ||
...result.data, | ||
JakobJingleheimer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
exists: result.data !== 'none', | ||
pjsonPath: result.path, | ||
}; | ||
} | ||
|
||
/** | ||
* @deprecated Expected to be removed in favor of `read` in the future. | ||
* Behaves the same was as `read`, but appends package.json to the path. | ||
* @param {string} requestPath | ||
* @return {PackageConfig} | ||
* @return {RecognizedPackageConfig} | ||
*/ | ||
function readPackage(requestPath) { | ||
// TODO(@anonrig): Remove this function. | ||
return read(resolve(requestPath, 'package.json')); | ||
return read(path.resolve(requestPath, 'package.json')); | ||
} | ||
|
||
/** | ||
* Get the nearest parent package.json file from a given path. | ||
* Return the package.json data and the path to the package.json file, or undefined. | ||
* @param {string} checkPath The path to start searching from. | ||
* @returns {undefined | {data: import('typings/internalBinding/modules').PackageConfig, path: string}} | ||
* @param {URL['href'] | URL['pathname']} startLocation The path to start searching from. | ||
* @param {boolean} everything Whether to include the full contents of the package.json. | ||
* @returns {undefined | DeserializedPackageConfig<everything>} | ||
*/ | ||
function getNearestParentPackageJSON(checkPath) { | ||
const result = modulesBinding.getNearestParentPackageJSON(checkPath); | ||
function getNearestParentPackageJSON(startLocation, everything = false) { | ||
let startPath = URL.canParse(startLocation) ? fileURLToPath(startLocation) : startLocation; | ||
|
||
validateString(startPath, 'startPath'); | ||
validateBoolean(everything, 'everything'); | ||
|
||
if (!path.isAbsolute(startPath)) { | ||
throw new ERR_INVALID_ARG_VALUE( | ||
'startLocation', | ||
startLocation, | ||
ArrayPrototypeJoin([ | ||
'must be a fully resolved location. To use a relative location, first wrap with', | ||
'`import.meta.resolve(startLocation)` in ESM', | ||
'or', | ||
'`path.resolve(__dirname, startLocation) in CJS', | ||
], ' '), | ||
); | ||
} | ||
|
||
if (result === undefined) { | ||
return undefined; | ||
if (!StringPrototypeEndsWith(startPath, path.sep)) { startPath += path.sep; } | ||
|
||
if (everything) { | ||
const result = modulesBinding.getNearestRawParentPackageJSON(startPath); | ||
JakobJingleheimer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this also need to be null prototyped There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aduh95 please advise 🙏 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, it's user-facing |
||
data: { | ||
__proto__: null, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having null-prototype object returned to the object feels weird There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. I did it for consistency between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That feels like another reason not to expose There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason is because we've already done the work, and handled the caching. Doing it this way also provides better performance. Users having to do exactly the same thing is the whole reason we as engineers create utilities, and there is basically no chance any user would not subsequently parse it. Freezing seems like a good idea. |
||
...JSONParse(result?.[0]), | ||
}, | ||
path: result?.[1], | ||
}; | ||
} | ||
|
||
const data = deserializePackageJSON(checkPath, result); | ||
const result = modulesBinding.getNearestParentPackageJSON(startPath); | ||
|
||
// Path should be the root folder of the matched package.json | ||
// For example for ~/path/package.json, it should be ~/path | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: Why does this statement doesn't hold anymore? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
const path = StringPrototypeSlice(data.pjsonPath, 0, StringPrototypeLastIndexOf(data.pjsonPath, sep)); | ||
if (result === undefined) { | ||
return undefined; | ||
} | ||
|
||
return { data, path }; | ||
return deserializePackageJSON(startPath, result); | ||
} | ||
|
||
/** | ||
* Returns the package configuration for the given resolved URL. | ||
* @param {URL | string} resolved - The resolved URL. | ||
* @returns {import('typings/internalBinding/modules').PackageConfig} - The package configuration. | ||
* @returns {RecognizedPackageConfig & { | ||
* exists: boolean, | ||
* pjsonPath: DeserializedPackageConfig['path'], | ||
* }} - The package configuration. | ||
*/ | ||
function getPackageScopeConfig(resolved) { | ||
const result = modulesBinding.getPackageScopeConfig(`${resolved}`); | ||
|
||
if (ArrayIsArray(result)) { | ||
return deserializePackageJSON(`${resolved}`, result); | ||
const { data, path } = deserializePackageJSON(`${resolved}`, result); | ||
return { | ||
__proto__: null, | ||
...data, | ||
pjsonPath: path, | ||
}; | ||
} | ||
|
||
// This means that the response is a string | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it is a good idea to introduce yet-another way of loading a JSON file, wouldn't it be simpler to return the path and let the user deal with reading/parsing the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that (there's another PR I opened and closed that exposed
findPackageJSON
, which did exactly that). We're already reading the pjson and caching it, so IMO that would force a de-op as well as force the user to manually do what we're already doing. So, why?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think returning the parsed data means every time we consider parsing another field in the package.json, it needs to be surfaced into this API, even though that field may not even be useful for tools, but we'll end up paying the serialization/deserialization cost even though internally only selected places need selected fields. IMO even just returning the string would be better than this. Users need this for the lookup algorithm, they tend to have other fields to parse anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I recommend a different naming and API surface?
module.findPackageJson(path, { includeContents: true })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, @joyeecheung I'm not sure I follow in your message. I think you've misunderstood the behaviour here: internally, we do not use the full version—we do use
getNearestParentPackageJSON
but do not seteverything
totrue
. Wheneverything
is nottrue
, only the select fields we use internally are parsed. That has not changed in this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anonrig if you recall, when we were working on this a few days ago, that did not make sense due to how the C++ worked, which is what caused me to abandon the
findPackageJSON
+getPackageJSON
route in the first place.Regarding "find" +
includeContents
, this seems counter-intuitive to me: I expect something called "find" to only locate, not retrieve. I could perhaps see the inversemodule.getPackageJSON(path, { includeContents: false })
I do not like either option though because the shape of the return is changed significantly in a foot-gun way:
(I simplified the 2nd arg for brevity, not necessarily to say we shouldn't use an object)
I suppose it could always return an object for consistency, but that seems like a clumsy compromise
If we were to do this, I think exposing 2 different utils (
findPackageJSON
+getPackageJSON
) would be better.