refactor(many): convert ui-scripts to TypeScript and add tests#2578
refactor(many): convert ui-scripts to TypeScript and add tests#2578ToMESSKa wants to merge 1 commit into
Conversation
ToMESSKa
force-pushed
the
INSTUI-5038-convert-ui-scripts-to-type-script
branch
from
5dc449d to
490034f
Compare
|
Visual regression report✅ No changes.
Baselines come from the |
| * @param bin {string} The binary name, e.g. `pwd` | ||
| * @param args {string[]} command arguments | ||
| * @param envVars {Object.<string, string>} Environment variables | ||
| * @param [envVars] {Object.<string, string>} Environment variables |
There was a problem hiding this comment.
babel.ts calls getCommand(specifyCJSFormat, []) with only 2 arguments — after the TS conversion TypeScript started enforcing the JSDoc that said all 3 args were required, so I marked the 3rd one optional.
| /** | ||
| * @param [commitIsh] {string} | ||
| * @param [allPackages] {any[]} | ||
| */ |
There was a problem hiding this comment.
npm.ts calls pkgUtils.getChangedPackages() with no arguments — after the TS conversion TypeScript started enforcing the JSDoc that said all 2 args were required, so I marked them optional.
|
|
||
| /** | ||
| * @param [options] {readPkgUp.NormalizeOptions} | ||
| */ |
There was a problem hiding this comment.
npm.ts calls pkgUtils.getPackage() with no arguments — after the TS conversion TypeScript started enforcing the JSDoc that said the arg was required, so I marked it optional.
| * Reads a package.json | ||
| * @param options {readPkgUp.NormalizeOptions} | ||
| * @param [options] {readPkgUp.NormalizeOptions} | ||
| * @returns {readPkgUp.NormalizedPackageJson} |
There was a problem hiding this comment.
bump.ts, publish.ts, tag.ts, and deprecate.ts all call pkgUtils.getPackageJSON() with no arguments — after the TS conversion TypeScript started enforcing the JSDoc options as required, so I marked it optional.
| * Generates a list of all versioned components based on the @instructure/ui meta package | ||
| * Run with: node --experimental-strip-types scripts/generateVersionedExports.ts | ||
| * Run with: node scripts/generateVersionedExports.ts | ||
| * from the packages/ui-codemods directory. |
There was a problem hiding this comment.
Node 22.18+ can natively run .ts files without the --experimental-strip-types flag, so it was removed it from packages/ui-scripts/lib/index.ts, packages/ui-codemods/package.json, and the packages/ui-codemods/scripts/generateVersionedExports.ts. I also bumped engines.node in the root package.json to >=22.18.0 to enforce the minimum.
| "ts:check": "tsc -p tsconfig.build.json --noEmit --emitDeclarationOnly false", | ||
| "generate:versioned-exports": "node --experimental-strip-types scripts/generateVersionedExports.ts" | ||
| "generate:versioned-exports": "node scripts/generateVersionedExports.ts" | ||
| }, |
There was a problem hiding this comment.
Node 22.18+ can natively run .ts files without the --experimental-strip-types flag, so it was removed it from packages/ui-scripts/lib/index.ts, packages/ui-codemods/package.json, and the packages/ui-codemods/scripts/generateVersionedExports.ts. I also bumped engines.node in the root package.json to >=22.18.0 to enforce the minimum.
| if (jspaths.length) { | ||
| runCommandSync('eslint', jspaths) | ||
| runCommandSync('eslint', argv.fix ? [...jspaths, '--fix'] : jspaths) | ||
| } |
There was a problem hiding this comment.
The --fix option was declared in line 31 but never used, so it was fixed this and tests added.
| "engines": { | ||
| "node": ">=22", | ||
| "node": ">=22.18.0", | ||
| "npm": "Use pnpm instead.", |
There was a problem hiding this comment.
Node 22.18+ can natively run .ts files without the --experimental-strip-types flag, so it engines.node so it was bumped >=22.18.0 to enforce the minimum.
| runGitCommand(['config', 'user.email', 'instui-dev@instructure.com']) | ||
| runGitCommand(['config', 'user.name', 'instructure-ui-ci']) | ||
| runGitCommand(['config', 'user.email', EMAIL]) | ||
| runGitCommand(['config', 'user.name', USERNAME]) |
There was a problem hiding this comment.
The USERNAME and EMAIL constants were declared at the top of the file but never used and TypeScript flagged them. Connected them to their use sites.
| // we could add them as imports from @token-studio/types if needed | ||
| case 'boolean': | ||
| ret += 'true' | 'false' | ||
| ret += "'true' | 'false'" |
There was a problem hiding this comment.
This showed and error after the TS conversion, I think it was meant to append the string 'true' | 'false' to ret, just like I did for textDecoration one line below, but it did not work. Added a test for this.
| const configFile = path.join(process.cwd(), argv.config) | ||
| const configFileURL = pathToFileURL(configFile) | ||
| const config = await import(configFileURL) | ||
| const config = await import(configFileURL.href) |
There was a problem hiding this comment.
TypeScript's dynamic-import() signature only allows string, so after the conversion it errored. I changed it to await import(configFileURL.href) which passes the URL as string.
| import readline from 'node:readline/promises' | ||
|
|
||
| import pkgUtils from '@instructure/pkg-utils' | ||
| import * as pkgUtils from '@instructure/pkg-utils' |
There was a problem hiding this comment.
TypeScript checked the package's actual .d.ts and errored because there's no default to import.
| */ | ||
|
|
||
| import pkgUtils from '@instructure/pkg-utils' | ||
| import * as pkgUtils from '@instructure/pkg-utils' |
There was a problem hiding this comment.
TypeScript checked the package's actual .d.ts and errored because there's no default to import.
| */ | ||
|
|
||
| import pkgUtils from '@instructure/pkg-utils' | ||
| import * as pkgUtils from '@instructure/pkg-utils' |
There was a problem hiding this comment.
TypeScript checked the package's actual .d.ts and errored because there's no default to import.
| */ | ||
| import semver from 'semver' | ||
| import pkgUtils from '@instructure/pkg-utils' | ||
| import * as pkgUtils from '@instructure/pkg-utils' |
There was a problem hiding this comment.
TypeScript checked the package's actual .d.ts and errored because there's no default to import.
|
|
||
| import { execSync } from 'node:child_process' | ||
| import pkgUtils from '@instructure/pkg-utils' | ||
| import * as pkgUtils from '@instructure/pkg-utils' |
There was a problem hiding this comment.
TypeScript checked the package's actual .d.ts and errored because there's no default to import.
| */ | ||
|
|
||
| import pkgUtils from '@instructure/pkg-utils' | ||
| import * as pkgUtils from '@instructure/pkg-utils' |
There was a problem hiding this comment.
TypeScript checked the package's actual .d.ts and errored because there's no default to import.
| if (error.code !== 'ENOENT') { | ||
| if ( | ||
| !(error instanceof Error && 'code' in error && error.code === 'ENOENT') | ||
| ) { |
There was a problem hiding this comment.
instanceof Error checks was added because TypeScript was complaining.
| error( | ||
| `Failed to resolve ${sourceTokens}: ${ | ||
| err instanceof Error ? err.message : String(err) | ||
| }` |
There was a problem hiding this comment.
instanceof Error checks was added because TypeScript was complaining.
| error( | ||
| `Failed to resolve ${outputPackage}: ${ | ||
| err instanceof Error ? err.message : String(err) | ||
| }` |
There was a problem hiding this comment.
instanceof Error checks was added because TypeScript was complaining.
| } | ||
| } catch (e) { | ||
| error(e.stack ? e.stack : e) | ||
| error(e instanceof Error && e.stack ? e.stack : e) |
There was a problem hiding this comment.
instanceof Error checks was added because TypeScript was complaining.
ToMESSKa
force-pushed
the
INSTUI-5038-convert-ui-scripts-to-type-script
branch
from
490034f to
ac8efca
Compare
1 participant
INSTUI-5038
ISSUE:
TEST PLAN:
TODO: