Convert project to ES modules (align tooling with the `service` repo) by bickelj-agent · Pull Request #276 · PhilanthropyDataCommons/data-scripts

bickelj-agent · 2026-07-22T18:51:31Z

This branch moves data-scripts from CommonJS to ES modules, in two reviewable commits, and then aligns its tooling with the service repository. Authored by GLM-5.2.

Why

data-scripts predates the shared, stricter tooling standardized in the service repository. Converting it to ESM in one step, then aligning to that tooling in a second step, keeps each change small and reviewable and lets the ESM conversion stand on its own.

Commit 1 — Convert project to ES modules

Minimal change required by ESM only:

Sets "type": "module", so Node resolves .js files as ESM. The already-present nodenext module setting then requires explicit .js suffixes on relative imports, and running the scripts from src/ needs a loader mapping those suffixes back to their .ts source.
Adds the ts-node ESM register hook the dev scripts now import, and the .js suffixes on relative imports.
Keeps the legacy ESLint config and single tsconfig.json, since they already use nodenext and still lint the ESM source.

Commit 2 — Align tooling with the `service` repository

Brings data-scripts in line so both repos lint, format, type-check, and test the same way:

Flat ESLint 9 config on eslint-config-love; Prettier; split build/type-check tsconfig; Jest in ESM mode; Node 24 engines; .npmrc; 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.

Verification

npm test — 2 suites, 9 tests passed.
npm run lint — eslint ./src --max-warnings=0, prettier --check, and tsc --noEmit all clean.
No whitespace-only churn vs main; the pre-existing scripts are Prettier-ignored to avoid polluting future git blame.

Generated-by: GLM-5.2

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

bickelj · 2026-07-22T18:58:41Z

I was able to run the charity navigator lookupAll and updateAll commands successfully on each of these commits using the test environment. This PR was basically go bring the repo up to speed for further development on it. I don't know how to do a CJS->ESM conversion myself so I had GLM-5.2 do it, looks pretty minimally invasive.

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

Copilot

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", .js import 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.

bickelj · 2026-07-22T19:07:13Z

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

You're missing some newlines, GLM.

bickelj

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

bickelj

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.

bickelj · 2026-07-23T18:06:20Z

  logger.warn('Have a `pdc-admin` create a source because only administrators may be able.');
  // The following may not succeed, doesn't succeed as of this writing.
-  return postSource(baseUrl, token, {
+  return await postSource(baseUrl, token, {


Why should we await here? Doesn't the function signature say this function returns a Promise and not a Source?

await is required here: postSource is now declared async because eslint-config-love's promise-function-async requires any function returning a Promise to be async, and return-await: 'always' then requires return await of that promise inside the async getOrCreateSource. The caller still receives a Promise<Source>.

— GLM-5.2

bickelj · 2026-07-23T18:09:09Z

-        Object.entries(baseFieldMap).map(async ([cnAttributeName, baseFieldShortCode]) => {
-          const cnAttribute = e[cnAttributeName as keyof NonprofitPublic];
-          /* eslint-disable-next-line @typescript-eslint/no-unnecessary-condition --
+    await Promise.all(


Was this new wrapper in response to a lint rule?

The Promise.all wrapper was added in this PR to satisfy no-floating-promises — main left the nonprofits.map(async ...) / Object.entries(...).map(async ...) arrays unawaited (fire-and-forget). But awaiting them concurrently caused the axios ETIMEDOUT you reported on the 2nd+ POSTs to /changemakerFieldValues. This push replaces it with a sequential for...of loop that awaits each POST (with no-await-in-loop disabled), which satisfies no-floating-promises without saturating the API.

— GLM-5.2

bickelj · 2026-07-23T18:14:14Z

+    const fields: BaseField[] = response.data;
+    const label = `${funder}: field label`;
+    const pos = `${funder}: form position`;
+    Promise.resolve(


Shouldn't this still be Promise.all?

Promise.all is not viable here: @typescript-eslint/await-thenable rejects passing an iterable of non-Promise (non-Thenable) values to a promise aggregator, and the .map callbacks are synchronous (they return plain ApplicationFormField | undefined values, not promises). Promise.resolve wraps the synchronous array so the subsequent .then chain runs, matching the original behavior.

— GLM-5.2

bickelj · 2026-07-23T18:15:37Z

-  if (typeof dataType !== 'string') {
-    throw new AssertionError({ message: 'Expected dataType to be a string' });
-  }
+Promise.resolve(


Again shouldn't this still be Promise.all?

Same as generateApplicationFormJson: the .map callbacks are synchronous (return plain SQL strings), so Promise.all would trip @typescript-eslint/await-thenable ("iterable of non-Promise values passed to promise aggregator"). Promise.resolve is the correct wrapper for the synchronous array feeding the .then chain.

— GLM-5.2

bickelj · 2026-07-23T18:18:55Z

-    '/baseFields',
-    {
+const getBaseFields = async (baseUrl: string, token: AccessTokenSet): Promise<BaseFieldBundle> =>
+  await callPdcApi<BaseFieldBundle>(baseUrl, '/baseFields', 'get', {


Why should we await here when the function signature indicates the caller of getBaseFields expects a Promise<BaseFieldBundle> and can itself do the await?

Same question for the next few function signatures and bodies too.

Both changes are mandated by eslint-config-love: promise-function-async requires any function that returns a Promise (here Promise<BaseFieldBundle>) to be declared async, and return-await: 'always' then requires return await of that promise inside the async function. main's non-async getBaseFields = (...) => (callPdcApi(...)) returned a Promise without being async, which the rule now disallows. The same applies to the sibling getters/posters.

— GLM-5.2

bickelj · 2026-07-23T18:20:32Z

 const requestTimeoutMs = 60000;

-const jsonSubstring = (json: object) => JSON.stringify(json).substring(0, JSON_SUBSTRING_LENGTH);
+const jsonSubstring = (json: object): string => JSON.stringify(json).substring(0, JSON_SUBSTRING_LENGTH);


Is there a lint rule requiring us to declare the return type of the function? Why not leave it implicit? I see this for a bunch of other functions too.

Yes — @typescript-eslint/explicit-function-return-type (enabled by eslint-config-love) requires explicit return types here. allowExpressions: true only exempts inline expressions, not const-bound arrow functions (treated as named declarations); removing the annotations produces 11 explicit-function-return-type errors.

— GLM-5.2

bickelj · 2026-07-23T18:23:31Z

      if (fieldValue === undefined) {
        throw new Error(`Undefined value for key '${key}' in row`);
-      } else if (fieldValue !== '') {
+      } else if (fieldValue === '') {


What was wrong with the previous construction? If a linter rule didn't require the change, leave it how it was.

Two rule-required changes here, both verified: (1) const { [key]: fieldValue } = row; instead of row[key] is required by @typescript-eslint/prefer-destructuring (eslint-config-love sets enforceForRenamedProperties: true) — reverting to row[key] errors. (2) The branch reorder (positive === '' in the else if, push in the else) is required by no-negated-condition — main's else if (fieldValue !== '') errors with "Unexpected negated condition." The logic (undefined → throw, '' → skip-log, else → push) is unchanged.

— GLM-5.2

Copilot

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, but lookupFromPdc doesn't define an ein argument; the relevant values are the derived validEins array in this handler. The current log will print undefined.

    } 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

bickelj · 2026-07-23T20:32:13Z

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.

bickelj · 2026-07-23T23:07:50Z

I tested updateAll and lookupFromPdc using ddcb93d and both CN scripts worked. Nice work, GLM-5.2.

bickelj-agent changed the title ~~Convert project to ES modules (align tooling with the service repo)~~ Convert project to ES modules (align tooling with the service repo) Jul 22, 2026

bickelj requested review from Copilot and slifty July 22, 2026 18:56

Copilot started reviewing on behalf of bickelj July 22, 2026 18:57 View session

bickelj requested a review from hminsky2002 July 22, 2026 18:58

Copilot AI reviewed Jul 22, 2026

View reviewed changes

bickelj mentioned this pull request Jul 22, 2026

Support pagination for Charity Navigator import #275

Open

bickelj reviewed Jul 23, 2026

View reviewed changes

Comment thread .editorconfig Outdated

Comment thread .npmrc Outdated

Comment thread .prettierignore Outdated

bickelj-agent force-pushed the convert-cjs-to-esm branch from 6f9b24b to 51c31ee Compare July 23, 2026 14:28

bickelj requested changes Jul 23, 2026

View reviewed changes

Comment thread .gitignore Outdated

Comment thread .prettierignore Outdated

Comment thread .prettierignore Outdated

Comment thread eslint.config.mjs Outdated

Comment thread package.json Outdated

bickelj mentioned this pull request Jul 23, 2026

Bump openid-client from 5.6.5 to 6.8.4 #259

Open

bickelj-agent force-pushed the convert-cjs-to-esm branch from 51c31ee to 1dbeb5c Compare July 23, 2026 17:26

bickelj reviewed Jul 23, 2026

View reviewed changes

bickelj requested a review from Copilot July 23, 2026 18:27

Copilot started reviewing on behalf of bickelj July 23, 2026 18:28 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread src/charityNavigator.ts

Comment thread src/ein.ts

bickelj-agent force-pushed the convert-cjs-to-esm branch from 1dbeb5c to ddcb93d Compare July 23, 2026 20:22

Uh oh!

Conversation

bickelj-agent commented Jul 22, 2026

Why

Commit 1 — Convert project to ES modules

Commit 2 — Align tooling with the service repository

Verification

Uh oh!

bickelj commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

bickelj commented Jul 22, 2026

Uh oh!

bickelj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bickelj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bickelj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

bickelj commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bickelj commented Jul 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Commit 2 — Align tooling with the `service` repository

bickelj commented Jul 22, 2026 •

edited

Loading

bickelj commented Jul 23, 2026 •

edited

Loading