chore(test): use import attributes for json files#3993
Conversation
tido64
requested review from
JasonVMo,
Saadnajmi and
acoates-ms
as code owners
github-actions
bot
added
the
chore
| // `ERR_UNKNOWN_FILE_EXTENSION` when encountering `.ts` files. | ||
| const version = Number(process.version.slice(1).split(".")[0]); | ||
| return version < 22 || manifest.includes('"type": "commonjs"'); | ||
| return manifest.type === "commonjs"; |
There was a problem hiding this comment.
With the ability to run .ts in node directly do we still need tsx?
There was a problem hiding this comment.
Last I tried, it failed on commonjs modules, which is why we have this check. But I can check again to make sure.
| return manifest; | ||
| async function importManifest(cwd) { | ||
| const url = pathToFileURL(path.join(cwd, "package.json")); | ||
| const manifest = await import(url.href, { with: { type: "json" } }); |
There was a problem hiding this comment.
I believe the main tradeoff with using import vs. readFileSync is that with import the file is loaded into the module cache, though it remains a mutable object, meaning if you edit a field in the manifest it will be reflected everywhere you also import, but not if you read the file. This is generally great in normal situations, but doesn't work as well in situations where we would write package.json files (align-deps, yarn constraints, etc.)
There was a problem hiding this comment.
require('./package.json') is the same as import in terms of module cache behavior. We should probably have a consistent philosophy of how we approach this in the repo. This change should be fine though, script command usage makes sense.
There was a problem hiding this comment.
I think in general, you'll want to read the file and then parse it to ensure no side effects. We use import here because it's more concise and the command basically exits in this scope.
2 participants
Description
This replaces a
fs.readFileSynccall withimport(). It changes nothing, just makes things neater.Test plan
n/a