Skip to content

chore(test): use import attributes for json files #3993

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

Merged
tido64 merged 1 commit into from
Feb 25, 2026
+12 −13
Merged

chore(test): use import attributes for json files #3993

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 12 additions & 13 deletions scripts/src/commands/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,35 @@
// @ts-check
import { Command, Option } from "clipanion";
import * as fs from "node:fs";
import * as path from "node:path";
import { pathToFileURL } from "node:url";
import { execute, runScript } from "../process.js";

/**
* @param {string} cwd
* @returns {string}
* @returns {Promise<Record<string, unknown>>}
*/
function readManifest(cwd = process.cwd()) {
const options = /** @type {const} */ ({ encoding: "utf-8" });
const manifest = fs.readFileSync(cwd + "/package.json", options);
return manifest;
async function importManifest(cwd) {
const url = pathToFileURL(path.join(cwd, "package.json"));
const manifest = await import(url.href, { with: { type: "json" } });
Copy link
Collaborator

@JasonVMo JasonVMo Feb 24, 2026

Choose a reason for hiding this comment

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

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.)

Copy link
Collaborator

@JasonVMo JasonVMo Feb 24, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@tido64 tido64 Feb 25, 2026

Choose a reason for hiding this comment

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

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.

return manifest.default ?? manifest;
}

/**
* @param {string} manifest
* @param {Record<string, unknown>} manifest
* @param {string} cwd
* @returns {boolean}
*/
function useJest(manifest, cwd) {
return manifest.includes('"jest"') || fs.existsSync(cwd + "/jest.config.js");
return Boolean(manifest.jest) || fs.existsSync(cwd + "/jest.config.js");
}

/**
* @param {string} manifest
* @param {Record<string, unknown>} manifest
* @returns {boolean}
*/
function useTsx(manifest) {
// Always use `tsx` with Node versions <22 otherwise it'll throw
// `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";
Copy link
Collaborator

@JasonVMo JasonVMo Feb 24, 2026

Choose a reason for hiding this comment

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

With the ability to run .ts in node directly do we still need tsx?

Copy link
Member Author

@tido64 tido64 Feb 25, 2026

Choose a reason for hiding this comment

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

Last I tried, it failed on commonjs modules, which is why we have this check. But I can check again to make sure.

}

export class TestCommand extends Command {
Expand All @@ -48,7 +47,7 @@ export class TestCommand extends Command {

async execute() {
const cwd = process.cwd();
const manifest = readManifest(cwd);
const manifest = await importManifest(cwd);

if (useJest(manifest, cwd)) {
return await runScript("jest", "--passWithNoTests", ...this.args);
Expand Down
Loading