Skip to content

refactor: restructure package.json for better module support and update scripts#136

Open
Kolezhniuk wants to merge 7 commits into
masterfrom
feat/esm-migration
Open

refactor: restructure package.json for better module support and update scripts#136
Kolezhniuk wants to merge 7 commits into
masterfrom
feat/esm-migration

Conversation

@Kolezhniuk
Copy link
Copy Markdown
Contributor

chore: remove rollup configuration as Vite is now used for building

feat: add fastfile.browser.js for browser compatibility with memory file operations

refactor: simplify fastfile.js by normalizing options and improving error handling

test: migrate tests from Mocha/Chai to Vitest and update assertions

test: add browser-specific tests for fastfile.browser.js functionality

chore: add Vite configuration for building both node and browser versions

…te scripts

chore: remove rollup configuration as Vite is now used for building

feat: add fastfile.browser.js for browser compatibility with memory file operations

refactor: simplify fastfile.js by normalizing options and improving error handling

test: migrate tests from Mocha/Chai to Vitest and update assertions

test: add browser-specific tests for fastfile.browser.js functionality

chore: add Vite configuration for building both node and browser versions
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR migrates the project’s build/test tooling from Rollup + Mocha/Chai to Vite + Vitest, introduces a dedicated browser entrypoint for in-memory usage, and updates packaging/exports to support both Node and browser consumers.

Changes:

  • Add Vite config to build separate Node (CJS) and browser (ESM/IIFE) bundles, and configure multi-project Vitest (Node + Playwright browser).
  • Introduce src/fastfile.browser.js and browser-focused tests for mem/bigMem behavior and URL-based readExisting.
  • Update package.json exports/scripts and migrate existing tests from Mocha/Chai to Vitest; update CI accordingly.

Reviewed changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
vite.config.js Defines dual-mode Vite library builds and multi-project Vitest (node-esm/node-cjs/browser-esm).
package.json Updates exports for node/browser, replaces mocha/rollup scripts with vitest/vite scripts, adds new devDependencies.
src/fastfile.js Refactors option normalization/dispatch and simplifies type handling.
src/fastfile.browser.js Adds a browser-specific entry that supports mem/bigMem + URL fetch, and blocks file I/O.
test/osfile.test.js Migrates assertions to Vitest.
test/memfile.test.js Migrates assertions to Vitest.
test/bigmemfile.test.js Migrates assertions to Vitest.
test/test.js Migrates some assertions to Vitest (integration-style test).
test/fastfile.browser.test.js Adds browser-focused coverage (including Playwright run) for the new browser entry.
eslint.config.js Introduces flat ESLint config aligned with the new tooling.
eslint.config.mjs Adds an additional ESLint config variant (currently problematic due to missing dependency).
.github/workflows/ci.yml Updates CI to a single Ubuntu job and installs Playwright before running tests.
rollup.cjs.config.js Removes legacy Rollup configuration.
.eslintrc.cjs Removes legacy ESLint config.
build/node/main.cjs Adds generated Node CJS bundle output.
build/browser/browser.esm.js Adds generated browser ESM bundle output.
build/browser/browser.iife.js Adds generated browser IIFE bundle output.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/test.js
Comment on lines +49 to +53
await expect(async () => {
const f = await fastFile.readExisting(fileName);
await writeBigInt(f, Scalar.add(q,3), 0);
await f.close();
} catch (err) {
throwed = true;
}
assert(throwed == true);
});

it("should read the file", async () => {
const f = await fastFile.readExisting(fileName);
for (let i=0; i<1000000; i++) {
const n = await readBigInt(f, i*32);
if (Scalar.sub(n, q) != i) {
console.log(f);
console.log(Scalar.sub(n, q));
console.log(i);
}
assert(Scalar.sub(n, q) == i);
if ((i%100000) == 0) console.log("Reading: " + i);
}
await f.close();
}).rejects.toThrow();
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

expect(...).rejects expects a Promise, but an async function is being passed here. This will fail with a type error (or won’t actually assert the rejection). Call the async function to produce a Promise (or use a Promise-returning expression) before asserting .rejects.toThrow().

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Kolezhniuk please take a look

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Checked this empirically against Vitest 4 (the version this PR migrates to). Unlike Jest, Vitest's .rejects/.resolves modifiers accept a function, invoke it, and await the returned promise.

Control test to confirm it actually asserts:

// passes (throws) ✅
await expect(async () => { throw new Error("boom"); }).rejects.toThrow();
// FAILS as expected ✅ — "promise resolved 42 instead of rejecting"
await expect(async () => 42).rejects.toThrow();

So the assertion at L49–53 does genuinely assert the rejection. Leaving as-is.

Comment thread vite.config.js
Comment thread eslint.config.mjs Outdated
Comment on lines +6 to +35
import { FlatCompat } from "@eslint/eslintrc";

const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);
const compat = new FlatCompat({
baseDirectory: __dirname,
recommendedConfig: js.configs.recommended,
allConfig: js.configs.all
});

export default defineConfig([{
extends: compat.extends("eslint:recommended"),

languageOptions: {
globals: {
...globals.node,
...globals.mocha,
},

ecmaVersion: 2022,
sourceType: "module",
},

rules: {
indent: ["error", 4],
"linebreak-style": ["warn", "unix"],
quotes: ["error", "double"],
semi: ["error", "always"],
},
}]); No newline at end of file
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

eslint.config.mjs imports FlatCompat from @eslint/eslintrc, but @eslint/eslintrc is not present in devDependencies/package-lock.json. If ESLint picks up this config, linting will fail with a module-not-found error. Either add @eslint/eslintrc or remove/rename this file (especially since eslint.config.js is also present).

Suggested change
import { FlatCompat } from "@eslint/eslintrc";
const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);
const compat = new FlatCompat({
baseDirectory: __dirname,
recommendedConfig: js.configs.recommended,
allConfig: js.configs.all
});
export default defineConfig([{
extends: compat.extends("eslint:recommended"),
languageOptions: {
globals: {
...globals.node,
...globals.mocha,
},
ecmaVersion: 2022,
sourceType: "module",
},
rules: {
indent: ["error", 4],
"linebreak-style": ["warn", "unix"],
quotes: ["error", "double"],
semi: ["error", "always"],
},
}]);
const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);
export default defineConfig([
js.configs.recommended,
{
languageOptions: {
globals: {
...globals.node,
...globals.mocha,
},
ecmaVersion: 2022,
sourceType: "module",
},
rules: {
indent: ["error", 4],
"linebreak-style": ["warn", "unix"],
quotes: ["error", "double"],
semi: ["error", "always"],
},
},
]);

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

eslint.config.mjs was a leftover from the .eslintrc → flat-config migration. ESLint resolves eslint.config.js first (it takes priority over .mjs), so this file was never actually loaded — eslint.config.js is the active, complete config (build ignores + node/browser/vitest globals). Removed the dead file rather than pulling in @eslint/eslintrc. npm run lint passes clean.

Comment thread .github/workflows/ci.yml Outdated
Comment thread src/fastfile.browser.js Outdated
Comment on lines +12 to +13
if (typeof o !== "string") return o;
return { type: "mem", initialSize: initialSize || DEFAULT_CACHE_SIZE };
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

In the browser entry, passing a string to createOverride/createNoOverride is silently treated as { type: "mem" } (and the string value is ignored). This can mask accidental usage of file paths (e.g., code copied from the Node README) and make debugging harder. Consider throwing File I/O is not supported in the browser for string inputs to the create* APIs, and reserving string handling for readExisting(url) only.

Suggested change
if (typeof o !== "string") return o;
return { type: "mem", initialSize: initialSize || DEFAULT_CACHE_SIZE };
if (typeof o === "string") noFileSupport();
return o;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — createOverride/createNoOverride now throw File I/O is not supported in the browser for string inputs, matching the existing behavior of readWriteExisting. String handling stays reserved for readExisting(url).

Comment thread vite.config.js
Comment thread package.json
Comment on lines +6 to +10
"main": "./build/node/main.cjs",
"module": "./src/fastfile.js",
"exports": {
"import": "./src/fastfile.js",
"require": "./build/main.cjs"
".": {
"node": {
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

main/exports.require now point to ./build/node/main.cjs, but the repo still has the legacy build/main.cjs. If it’s no longer used, consider removing it (and standardizing on build/node / build/browser) to avoid keeping or publishing a stale bundle.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the stale build/main.cjs. main/exports.require already point to build/node/main.cjs.

@Kolezhniuk Kolezhniuk requested a review from OBrezhniev May 11, 2026 10:06
@Kolezhniuk Kolezhniuk marked this pull request as ready for review May 11, 2026 10:08
@Kolezhniuk Kolezhniuk requested a review from vmidyllic May 11, 2026 10:08
Comment thread .github/workflows/ci.yml Outdated
branches:
- main
pull_request:
on: [push, pull_request]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let's do it only on PR, or as it was

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Restored the original triggers: push on master + pull_request (it was firing on every push to any branch). Also brought back the OS/Node matrix that Copilot flagged above.

Copy link
Copy Markdown
Member

@OBrezhniev OBrezhniev left a comment

Choose a reason for hiding this comment

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

Please check copilot review, and my previous comment.

Comment thread test/test.js
Comment on lines +49 to +53
await expect(async () => {
const f = await fastFile.readExisting(fileName);
await writeBigInt(f, Scalar.add(q,3), 0);
await f.close();
} catch (err) {
throwed = true;
}
assert(throwed == true);
});

it("should read the file", async () => {
const f = await fastFile.readExisting(fileName);
for (let i=0; i<1000000; i++) {
const n = await readBigInt(f, i*32);
if (Scalar.sub(n, q) != i) {
console.log(f);
console.log(Scalar.sub(n, q));
console.log(i);
}
assert(Scalar.sub(n, q) == i);
if ((i%100000) == 0) console.log("Reading: " + i);
}
await f.close();
}).rejects.toThrow();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Kolezhniuk please take a look

Comment thread eslint.config.mjs Outdated
Comment on lines +6 to +35
import { FlatCompat } from "@eslint/eslintrc";

const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);
const compat = new FlatCompat({
baseDirectory: __dirname,
recommendedConfig: js.configs.recommended,
allConfig: js.configs.all
});

export default defineConfig([{
extends: compat.extends("eslint:recommended"),

languageOptions: {
globals: {
...globals.node,
...globals.mocha,
},

ecmaVersion: 2022,
sourceType: "module",
},

rules: {
indent: ["error", 4],
"linebreak-style": ["warn", "unix"],
quotes: ["error", "double"],
semi: ["error", "always"],
},
}]); No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

d5k9 and others added 3 commits June 4, 2026 15:22
- ci: restore OS (ubuntu/macos/windows) and Node (lts/*, lts/-1) matrix and
  scope push trigger to master + PR (keep playwright install step)
- vite: exclude browser test from node-esm project to avoid duplicate runs
- vite: add testTimeout/hookTimeout to browser-esm project
- browser: throw "File I/O is not supported" for string inputs to create*
  instead of silently creating an anonymous mem file
- chore: remove dead eslint.config.mjs (eslint.config.js is the active flat
  config) and stale build/main.cjs artifact

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants