Conversation
There was a problem hiding this comment.
Pull request overview
Updates the project’s TypeScript/Jest configuration and bumps multiple devDependencies, while also adjusting published package entrypoints intended to reflect the build outputs.
Changes:
- Add Jest type declarations to the Jest-specific tsconfig.
- Remove explicit
module/target/moduleResolutionfrom the roottsconfig.json. - Update
package.jsonentrypoint fields and upgrade several devDependencies (Jest, Rollup plugins, TypeScript, ESLint-related packages, etc.).
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tsconfig.test.jest.json | Adds types: ["jest"] to improve Jest test typechecking. |
| tsconfig.json | Removes explicit TS compilation settings (now relies on defaults). |
| package.json | Updates entrypoints (main/module/UMD fields) and upgrades devDependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5322505 to
a9eb5a0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "exports": { | ||
| "./package.json": "./package.json", | ||
| ".": { | ||
| "types": "./dist/index.d.ts", | ||
| "import": "./dist/esm/index.esm.mjs", | ||
| "require": "./dist/formats/index.cjs.js" | ||
| "import": "./dist/index.js", | ||
| "require": "./dist/formats/cjs/index.js" | ||
| } |
There was a problem hiding this comment.
With "type": "module", Node will treat dist/formats/cjs/index.js as ESM (because it is a .js file within an ESM package scope). That means the exports["."].require entry will likely throw ERR_REQUIRE_ESM for CommonJS consumers. Consider emitting the CJS build with a .cjs extension (and pointing require to that), or adding a dist/formats/cjs/package.json with { "type": "commonjs" } during build/publish so that require resolves to true CommonJS.
| "source": "src/index.ts", | ||
| "types": "dist/index.d.ts", | ||
| "sideEffects": false, | ||
| "files": [ | ||
| "dist" | ||
| ], | ||
| "exports": { | ||
| "./package.json": "./package.json", | ||
| ".": { | ||
| "types": "./dist/index.d.ts", | ||
| "import": "./dist/esm/index.esm.mjs", | ||
| "require": "./dist/formats/index.cjs.js" | ||
| "import": "./dist/index.js", | ||
| "require": "./dist/formats/cjs/index.js" |
There was a problem hiding this comment.
The top-level types field was removed. While the exports["."].types entry works for newer TypeScript/tooling, older TypeScript versions and some ecosystem tools still rely on the root-level types/typings field to find declarations. Consider re-adding "types": "./dist/index.d.ts" for broader compatibility (it can coexist with the exports map).
| "require": "./dist/formats/index.cjs.js" | ||
| "import": "./dist/index.js", | ||
| "require": "./dist/formats/cjs/index.js" | ||
| } |
There was a problem hiding this comment.
exports no longer exposes ./package.json. When exports is present, Node disallows pkg/package.json access unless explicitly exported, so this is a breaking change for consumers/tools that read package metadata via import/require('react-hook-webstorage/package.json'). If that access should remain supported, consider restoring "./package.json": "./package.json" under exports.
| } | |
| }, | |
| "./package.json": "./package.json" |
a9eb5a0 to
368b909
Compare
368b909 to
ebda722
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 10 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
Makefile:17
- With .prettierrc/.prettierignore removed in this PR, "prettier . --check" will fall back to Prettier defaults and traverse the whole repo. Given the current codebase uses single quotes, this validate step is likely to start failing unless the code is reformatted or the Prettier config is reintroduced/migrated (e.g. prettier.config.* or a "prettier" key in package.json) and generated dirs like dist/ are ignored.
./node_modules/.bin/eslint \
.
./node_modules/.bin/prettier \
. \
--check
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| files: ['**/*.test.*'], | ||
| languageOptions: { globals: { ...globals.jest } }, | ||
| ...ConfigJest.configs['flat/recommended'], | ||
| ...ConfigJest.configs['flat/style'], | ||
| }, |
There was a problem hiding this comment.
The Jest ESLint override only targets files matching "**/.test.", but this repo's Jest tests are ".spec.ts" and "jest..ts" (e.g. src/useWebStorage.spec.ts). As-is, Jest globals/rules won’t apply to existing tests and lint will likely fail or miss intended test-specific rules. Update the "files" globs to include the project’s actual test naming patterns.
| globalIgnores([ | ||
| 'coverage/', | ||
| 'example/', | ||
| 'tests/', | ||
| 'package-lock.json', | ||
| ]), |
There was a problem hiding this comment.
The flat-config ignores don’t include the generated "dist/" directory (previously ignored via .eslintignore, and still gitignored). If developers run build before validate, "eslint ." can end up linting generated output. Add "dist/" to globalIgnores (or otherwise ignore it) to keep linting focused on source.
Open ToDos