Convert to ES modules and align tooling and lint rules with the service repo#276
Convert to ES modules and align tooling and lint rules with the service repo#276bickelj-agent wants to merge 3 commits into
service repo#276Conversation
With `"type": "module"`, Node resolves `.js` files as ESM, so the already-present `nodenext` setting now requires explicit `.js` suffixes on relative imports, and running the scripts from `src/` needs a loader mapping those suffixes back to their `.ts` source. This change is only what ESM forces (the suffixes, the `ts-node` ESM register hook the dev scripts now import, and the type field) so it can stand alone and stay reviewable; the service repo's broader tooling is deferred to the next commit. The legacy ESLint config and single tsconfig are kept, since they already use `nodenext` and still lint the ESM source. Generated-by: GLM-5.2
service repo)
service repo)
|
I was able to run the charity navigator @slifty @hminsky2002 I don't think it would be best for the one who prompted this PR to also be the one to review this PR, so I would appreciate one of y'all's reviews. |
There was a problem hiding this comment.
Pull request overview
This pull request migrates data-scripts from CommonJS to ES modules and then aligns the repo’s TypeScript, linting/formatting, and test tooling with the stricter standards used in the service repository, while minimizing diff churn in pre-existing scripts.
Changes:
- Switch Node execution and internal imports to ESM (
"type": "module",.jsimport specifiers, ts-node ESM loader hook). - Split TypeScript configs into build vs dev/typecheck, and add Jest-based unit tests for key parsing helpers.
- Replace legacy ESLint config with ESLint 9 flat config + Prettier, plus editor/npm configuration alignment.
Reviewed changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Introduces build output settings (dist/) and declaration emit for ESM build. |
| tsconfig.dev.json | Adds a dev/typecheck-only TS config used by linting and tests. |
| src/postProposalVersions.ts | Updates Node core imports and ESM-relative import specifiers; replaces inline literals with named constants. |
| src/pdc-api.ts | Updates ESM import specifiers and makes type re-exports explicit. |
| src/oidc.ts | Switches Node core import to node: prefix and adopts type imports for ESM hygiene. |
| src/index.ts | Updates internal imports to .js specifiers required under Node ESM. |
| src/generateBaseFieldsInserts.ts | Converts Node core imports to node: prefix and updates internal imports to .js. |
| src/generateApplicationFormJson.ts | Updates internal imports to .js and corrects axios type-only import usage. |
| src/ein.unit.test.ts | Adds Jest unit tests for EIN validation behavior. |
| src/csv.unit.test.ts | Adds Jest unit tests for CSV row assertion behavior. |
| src/csv.ts | Switches assert import to node: prefix. |
| src/client.ts | Updates internal logger import to .js and uses type-only axios imports. |
| src/charityNavigator.ts | Updates Node/core and internal ESM imports; extracts JSON formatting constants. |
| src/candid.ts | Updates Node/core and internal ESM imports; extracts JSON/rate-limit constants. |
| register-ts-node-esm.mjs | Adds a Node --import hook to register ts-node/esm for running TS entrypoints under ESM. |
| package.json | Sets "type": "module", adds build/lint/format/test scripts, pins Node engine to 24, and updates dev tooling deps. |
| jest.config.cjs | Adds Jest configuration for ESM + ts-jest + .js specifier mapping. |
| eslint.config.mjs | Replaces legacy ESLint config with ESLint 9 flat config aligned to service tooling. |
| .prettierrc | Adds repo Prettier settings (single quotes, 120 print width). |
| .prettierignore | Avoids formatting churn on pre-existing scripts and legacy docs/configs. |
| .npmrc | Enables engine-strict to enforce the Node 24 engine constraint. |
| .gitignore | Adds local conversion-notes doc to ignore list. |
| .eslintrc.json | Removes legacy ESLint config in favor of flat config. |
| .editorconfig | Adds baseline editor formatting rules aligned with new tooling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
There is a raft of dependency updates pending, and I suspected those could be more easily merged/adapted if we changed this project to ESM, maybe that was incorrect, but that was also part of the impetus. |
bickelj
left a comment
There was a problem hiding this comment.
You're missing some newlines, GLM.
6f9b24b to
51c31ee
Compare
bickelj
left a comment
There was a problem hiding this comment.
I think there are two things to do here:
- Address the review feedback regarding removal of an md file and renaming another file, do this as amendments to the existing commit(s).
- A new third commit on this branch that enables the linting rules that the service repository enforces, along with all the updates needed to the code to pass those rules.
The data-scripts predate the shared, stricter tooling the service repository standardizes on, so this brings them in line so both repos lint, format, type-check, and test the same way: the flat ESLint 9 config on `eslint-config-love`, Prettier, the split build/type-check tsconfig, Jest in ESM mode, the Node 24 `engines` plus `.npmrc`, and the lockfile bumped to v3. The pre-existing scripts predate that strictness, so the rules that would force large ESM-unrelated refactors of them are disabled and those files are Prettier-ignored to keep the diff clean; only the small refactors the kept rules require (`node:` prefixes, named constants, `type`-only specifiers) are applied, with unit tests for `ein` and `csv` covering the parsing the ESM switch touched. Generated-by: GLM-5.2
51c31ee to
1dbeb5c
Compare
bickelj
left a comment
There was a problem hiding this comment.
Please remove the 120 character line limit from the prettier config in addition to answering the comments in-line. I am not super confident in my questions or suggestions, they may only be questions to answer or problems to fix.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/charityNavigator.ts:305
- This log references
args.ein, butlookupFromPdcdoesn't define aneinargument; the relevant values are the derivedvalidEinsarray in this handler. The current log will printundefined.
} else {
await writeFile(args.outputFile, JSON.stringify(charityNavResponse, null, JSON_SPACES));
logger.info(`Wrote CharityNavigator data for ${JSON.stringify(args.ein)} to ${JSON.stringify(args.outputFile)}`);
}
data-scripts had disabled the stricter eslint-config-love rules that the service repo enforces, to keep this conversion focused. This follow-up removes those overrides and the src/ Prettier-ignore entries so both repos lint and format identically, and adjusts the pre-existing scripts to comply. PR review feedback is folded in: empty-string argument guards are restored where strings are checked, a pre-existing args.ein logging bug is corrected, and the changemaker field-value POSTs are issued sequentially to avoid saturating the PDC API connection pool. Unit tests still pass with no other behavioral changes. Generated-by: GLM-5.2
1dbeb5c to
ddcb93d
Compare
|
This is stretching the limits of cline+together/GLM-5.2, 1.9M+ tokens over many rounds of refinement, with a 256K token limit, therefore many compactions, but it appears to be about to post the comments after I interrupted it and gave it a helpful summary of where we are. Edit: maybe didn't hit the limit after all, everything looks good. Edit2: perhaps it was GLM-5.2 stretching the limits of me, touche! |
|
I tested |
service repo)service repo
|
This Zulip conversation between @bickelj and @hminsky2002 may be helpful for grokking this PR. |
This branch moves
data-scriptsfrom CommonJS to ES modules and brings its tooling in line with theservicerepository, in three reviewable commits. Authored by GLM-5.2.Why
data-scriptspredates the shared, stricter tooling standardized in theservicerepository. As noted in this PR's comments, a raft of pending dependency updates should land more easily once the project is on ESM, so this PR converts to ESM first, aligns the tooling next, and finally enables the stricter lint and Prettier rules — keeping each step small and reviewable and letting the ESM conversion stand on its own.Commit 1 — Convert project to ES modules
Minimal change required by ESM only:
"type": "module", so Node resolves.jsfiles as ESM. The already-presentnodenextmodule setting then requires explicit.jssuffixes on relative imports, and running the scripts fromsrc/needs a loader mapping those suffixes back to their.tssource.ts-nodeESM register hook the dev scripts now import, and the.jssuffixes on relative imports.tsconfig.json, since they already usenodenextand still lint the ESM source.Commit 2 — Align tooling with the
servicerepositoryBrings
data-scriptsin line so both repos lint, format, type-check, and test the same way:eslint-config-love; Prettier; split build/type-checktsconfig; Jest in ESM mode; Node 24engines;.npmrc; lockfile bumped to v3.node:prefixes, named constants,type-only specifiers) are applied, with unit tests foreinandcsvcovering the parsing the ESM switch touched.Commit 3 — Enable the
service-repo lint and Prettier rulesRemoves the rule overrides and
src/Prettier-ignore entries deferred in commit 2, so both repos lint and format identically, and adjusts the pre-existing scripts to comply. PR review feedback is folded in:args.einlogging bug (the argument is theeinsarray, so it always loggedundefined); thelookupcommand now logsargs.einsandlookupFromPdclogsvalidEins.Promise.allof changemaker field-value POSTs with a sequentialfor...ofloop, fixing the axiosETIMEDOUTthat occurred on the second and later POSTs to/changemakerFieldValuesunder concurrent load.Verification
npm test— 2 suites, 9 tests passed.npm run lint—eslint ./src --max-warnings=0,prettier --check, andtsc --noEmitall clean.updateAllandlookupFromPdcrun successfully against the test environment on the final commit.main; the pre-existing scripts are Prettier-ignored in commit 2 only where needed, and those ignore entries are removed in commit 3.Generated-by: GLM-5.2