Skip to content

Convert to ES modules and align tooling and lint rules with the service repo - #276

Merged
bickelj merged 3 commits into
mainfrom
convert-cjs-to-esm
Jul 27, 2026
Merged

bickelj merged 3 commits into
mainfrom
convert-cjs-to-esm

Conversation

@bickelj-agent

@bickelj-agent bickelj-agent commented Jul 22, 2026 •

Copy link
Copy Markdown
Collaborator

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

Why

data-scripts predates the shared, stricter tooling standardized in the service repository. 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:

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

Commit 3 — Enable the service-repo lint and Prettier rules

Removes 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:

  • Restores empty-string argument guards where strings are checked.
  • Fixes a pre-existing args.ein logging bug (the argument is the eins array, so it always logged undefined); the lookup command now logs args.eins and lookupFromPdc logs validEins.
  • Replaces the concurrent Promise.all of changemaker field-value POSTs with a sequential for...of loop, fixing the axios ETIMEDOUT that occurred on the second and later POSTs to /changemakerFieldValues under concurrent load.

Verification

  • npm test — 2 suites, 9 tests passed.
  • npm run lint — eslint ./src --max-warnings=0, prettier --check, and tsc --noEmit all clean.
  • A reviewer confirmed updateAll and lookupFromPdc run successfully against the test environment on the final commit.
  • No whitespace-only churn vs 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

@bickelj-agent 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
bickelj requested review from Copilot and slifty July 22, 2026 18:56
@bickelj

bickelj commented Jul 22, 2026 •

Copy link
Copy Markdown
Collaborator

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.

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

bickelj commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

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 bickelj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You're missing some newlines, GLM.

Comment thread .editorconfig Outdated
Comment thread .npmrc Outdated
Comment thread .prettierignore Outdated

@bickelj bickelj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think there are two things to do here:

  1. Address the review feedback regarding removal of an md file and renaming another file, do this as amendments to the existing commit(s).
  2. 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.

Comment thread .gitignore Outdated
Comment thread .prettierignore Outdated
Comment thread .prettierignore Outdated
Comment thread eslint.config.mjs Outdated
Comment thread package.json Outdated

@bickelj bickelj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread src/charityNavigator.ts
Comment thread src/charityNavigator.ts Outdated
Comment thread src/charityNavigator.ts
Comment thread src/charityNavigator.ts Outdated
Comment thread src/charityNavigator.ts Outdated
Comment thread src/generateApplicationFormJson.ts
Comment thread src/generateBaseFieldsInserts.ts
Comment thread src/pdc-api.ts
Comment thread src/postProposalVersions.ts
Comment thread src/postProposalVersions.ts

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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)}`);
    }

Comment thread src/charityNavigator.ts
Comment thread src/ein.ts
@bickelj

bickelj commented Jul 23, 2026 •

Copy link
Copy Markdown
Collaborator

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!

@bickelj

bickelj commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

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

@bickelj-agent bickelj-agent changed the title Convert project to ES modules (align tooling with the service repo) Convert to ES modules and align tooling and lint rules with the service repo Jul 24, 2026
@kfogel

kfogel commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

This Zulip conversation between @bickelj and @hminsky2002 may be helpful for grokking this PR.

@bickelj
bickelj self-requested a review July 24, 2026 19:03

@bickelj bickelj left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM but I am the one who prompted this.

OK, I just "reviewed" with "comment" instead of "requested changes" but GH says I still have a requested change even though all those issues are marked resolved. What am I supposed to do here?

@bickelj
bickelj self-requested a review July 25, 2026 21:36

@bickelj bickelj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This review, where I comment but neither approve nor request changes, is intended to supersede the review where I previously requested changes. The comments are all resolved. It is not appropriate for me to "Approve" this because my agent opened the PR on my behalf (the agent account is a safety mechanism, not intended to give me the ability to "cheat" on PR approvals). I hope this clears the "Changes requested" flag.

@bickelj
bickelj dismissed their stale review July 27, 2026 15:49

This seems to be the only way to clear the "Changes requested" flag on this PR. I do not understand why this is the only way but hopefully GH will fix this someday.

@hminsky2002 hminsky2002 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Alright, having run this locally, and thrown an llm at it, here is my assessment: the changes are consistent with esm syntax, and all relevant keywords / imports have been updated. Something worth noting is that (I believe) the tests being added here are not integrated into the CI/CD workflow, which to me seems worth doing. Otherwise, I think this checks off the boxes, and the end result is that we can use features specific to ESM (like the lovable eslint flat config). The only change I am gonna request here is integrating the tests into the .github actions flow! Nice work in tandem with the llm, and pressing on important decisions, @bickelj

@bickelj

bickelj commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

@hminsky2002 A very wise suggestion! GLM-5.2 is on it.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated 1 comment.

Comment thread src/generateApplicationFormJson.ts

@hminsky2002 hminsky2002 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you for incorporating the tests into the workflow. Approved, with my only final thought being the more we can think through what a reasonable benchmark for a refactor of this type is, the better we'll be able to validate the success of it (i.e, in this case being able to run the flat pack of the lovable eslint config seems like a reasonable benchmark.)

@bickelj

bickelj commented Jul 27, 2026 •

Copy link
Copy Markdown
Collaborator

@hminsky2002 Many thanks for looking this over, yeah, I suppose there is an element of speculation here that conversion to ESM will make life easier for incorporating this bunch of deps. It may still be a pain either way or easy either way. The repo was out of date with regard to other tooling too.

Something that just occurred to me when looking over those dep updates: what about the dependabot config and does it reference deps we now dropped?

Phew, it's not making explicit reference to airbnb etc:

image

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
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
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
bickelj merged commit 433b7b4 into main Jul 27, 2026
6 checks passed
@bickelj
bickelj deleted the convert-cjs-to-esm branch July 27, 2026 20:12
@bickelj bickelj moved this to Done in Phase 2026 Jul 28, 2026
@bickelj bickelj removed this from Phase 2026 Jul 28, 2026
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.

5 participants