refactor: core CLI cleanup (dead code, dedup, testable units) + utils unit tests#439
Open
rin-st wants to merge 6 commits into
Open
refactor: core CLI cleanup (dead code, dedup, testable units) + utils unit tests#439rin-st wants to merge 6 commits into
rin-st wants to merge 6 commits into
Conversation
- node:test runner (no deps) via --experimental-strip-types - tiny resolve hook maps extensionless rollup-style imports to .ts - cover external-extensions url parsing, validateNpmName, templates/utils - convert type-only imports to `import type` on tested path - lint test/mjs files without type-aware rules Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- drop dead isConfigRegex (never matched POSIX paths, no config.json files) - prettier-format: correct error message, remove dead result.failed check - create-project-directory: fs.mkdir instead of spawning mkdir - install-packages: dedupe stdout/stderr chunking into one helper - merge-package-json: drop useless TODO .dev placeholder write (+ isDev param) - rename merge-pacakges.d.ts -> merge-packages.d.ts - type SolidityFrameworkChoices value as SolidityFramework | null Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- A1: pass copyOrLink as a param, drop module-level mutable let
- A2: single getExternalExtensionPath + SourcePaths object; stop
recomputing the external-extension path in 3 places
- A3: collectTemplateDescriptors / collectArgsFileUrl helpers replace
4 near-identical descriptor blocks and 3 args-lookup blocks
Output verified byte-identical (incl. symlinks) vs prior commit for
hardhat, none, and external-extension configs.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- A4: extract pure parseCliArgs into parse-cli-args.ts (unit-tested);
parse-arguments-into-options orchestrates small steps
(resolveExternalExtension / ensureCompatibleCreateEthVersion /
validateProjectName / resolveSolidityFrameworkChoices), keeping
process.exit out of the pure parser
- A6: export GLOBAL_ARGS_DEFAULTS from templates/utils.js + sync test
asserting it matches src/utils/consts
Output verified unchanged vs prior for hardhat, none, ext configs.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- drop obsolete solidity-framework TODO: 'generalize into general extensions' targets the extension system being deprecated - replace prettier-format TODO with a rationale: we run the generated project's own 'yarn format' on purpose (its config/plugins), so the suggested switch to the CLI's bundled prettier would be a regression Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- keep isConfigRegex config.json skip (only matched backslash/Windows paths but removing it changed extension-copy behaviour there); restores 1:1 parity with main - add engines.node >=22.6 (test script needs --experimental-strip-types) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tried to make some refactoring with claude and here is the result. Most of it are tests for utils. We didn't have them before but I think it's fine to have tests for utils so it would be more difficult to break them. On the other hand, utils are battle-tested and changes are rare, so tests could be redundant. What do you think?
Note 1: If we merge, I think we need to avoid combining those changes with next release (ui-tweaks, blockexplorer, yarn etc, I want to make it next version of sre-challenges)
Note 2: I also tried to refactor se-2 the same way, but the changes were so nitpicky, I think it's not even worth it to create PR there
Generated
TL;DR
Internal refactor of the core CLI only (
src/+templates/utils.js) — no changes to the generated app intemplates/base/**. Removes dead code and hidden mutable state, deduplicates the template-copy and arg-parsing logic, splits mixed concerns into small testable units, and adds the first unit tests (28, vianode:test, zero new deps).Generated-app output verified byte-identical (including symlinks) to
mainforhardhat,none, andexternal-extensionconfigs (non---devruns). Behaviour is preserved for normal use; two intentional behaviour-only deltas are flagged below (create-project-directorynested-path handling, and the--dev-only.devplaceholder).+~500 / −~280across ~23 files.Out of scope: the
solidityFramework→ "general extensions" generalization (old// TODO) — that targets the extension system that is being deprecated, so the misleading TODO was removed rather than acted on.Changes in detail
1. Tests (
test:commit)node:testsuite (no new dependencies).yarn testrunssrc/**/*.test.tsvia--experimental-strip-types.src/test/ts-extension-resolver.mjs, registered bysrc/test/register.mjs) maps./foo→./foo.ts/./foo/index.tsfor the test run only.getDataFromExternalExtensionArgument,getArgumentFromExternalExtensionOption),validateNpmName, andtemplates/utils.js(deepMerge,stringify,withDefaults,upperCaseFirstLetter).import typeon the tested dependency path (needed for type stripping; also general hygiene)..mjsfiles are linted without type-aware rules (they're excluded fromtsconfig).enginesinpackage.json. CI'slts/*covers this. No CI step is wired foryarn testyet — can add separately if wanted.2. Small fixes & cleanups
prettier-format.ts— corrected the wrong copy-pasted error message and removed the unreachableresult.failedcheck (execa rejects on failure).create-project-directory.ts— usefs.promises.mkdir(…, { recursive: true })instead of spawningexeca("mkdir", …); removed the same deadresult.failedcheck. Intentional behaviour delta: also fixes Windows (where themkdirbuiltin can't be spawned) and lets a nested project name (e.g.a/b/cwith missing parents) succeed, where the oldmkdirwithout-perrored.install-packages.ts— deduplicated the identical stdout/stderr buffer/chunk logic into one helper.merge-package-json.ts— dropped the dev-mode.devwrite that only emitted a literal"TODO: …"placeholder (and the now-unusedisDevparam + call sites).src/declarations/merge-pacakges.d.ts→merge-packages.d.ts.SolidityFrameworkChoicesvalue asSolidityFramework | nullinstead ofany.3.
copy-template-files.tsrestructurecopyOrLinkis now passed as a parameter instead of a module-level mutableletreassigned at call time (removes hidden global state).getExternalExtensionPath()plus aSourcePathsobject replaces the external-extension path computed (with the samedev ? … : …ternary) in three places.collectTemplateDescriptors()andcollectArgsFileUrl()helpers replace the four near-identical descriptor-building blocks and three near-identical args-lookup blocks.4. Arg-parsing split + global-args sync guard
parseCliArgs()intoparse-cli-args.ts(now unit-tested).parseArgumentsIntoOptionsis now a thin orchestrator over small steps:resolveExternalExtension,ensureCompatibleCreateEthVersion,validateProjectName,resolveSolidityFrameworkChoices.process.exitlives only in the version-compat gate, not in the parser.GLOBAL_ARGS_DEFAULTSis hand-copied betweensrc/utils/consts.tsandtemplates/utils.js. The two copies remain — this is not deduplicated to a single source, sinceutils.jsships into user projects and can't import the TS const without a build step. The change only exports thetemplates/utils.jscopy and adds a test that fails if the two definitions drift apart.5. Stale TODO cleanup
solidityFramework→ general-extensions TODO (extensions are being deprecated).prettier-formatTODO with a rationale: we intentionally run the generated project's ownyarn format(its config/plugins), so switching to the CLI's bundled prettier would be a regression.Verification
yarn type-check,yarn lint,yarn test(28 passing),yarn buildall green. Output parity confirmed by generating each config (non---dev) from this branch and frommainand diffing a file+symlink manifest (sha per file, target per symlink).Known issue (not fixed here)
The type check inside
withDefaults(templates/utils.js) is effectively a no-op — the "expected" type is derived from the already-merged value, so a provided arg always matches itself. Left as-is to avoid changing template behaviour repo-wide; worth a follow-up.Open question
No changeset included — changes are internal and behaviour-preserving for normal use (two intentional deltas noted above). Add one if the release flow expects it.
🤖 Generated with Claude Code