-
Notifications
You must be signed in to change notification settings - Fork 142
CLI TypeScript migration #2802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
CLI TypeScript migration #2802
Conversation
build:backend uses `tsc --noEmitOnError` to build the project. This causes built files to be placed directly next to .ts files in all projects, as tsc uses the tsconfig in the root of the project. (refer to https://stackoverflow.com/questions/64626846/typescript-tsc-not-picking-up-tsconfig-json-inside-a-subdirectory) We can update tsconfigs with project references to achieve this functionality, but let's leverage lerna for simplicity instead. This way, we use the sub-modules' already-defined `build` scripts to build the projects in a consistent way.
Scripts in root package.json refer to index.js in cli/ package. As part of TS migration, update scripts to refer to the index.js that is created after building the project. Add pre- scripts that build the project to ensure that the latest index.js file is used whenever a script is run. Extract cli path to a variable.
Typescript migration caused a regression where all debug logs were being printed to console. This is likely not intended behaviour as users of the cli would not want to see build output be cluttered Update cli console logger level to 'info' to retain expected output when using markbind-cli
deploy:netlify uses a pre- script in order to build the prerequisite backend module first. However deploy:netlify runs `npm run setup` in the main script - this cleans the directory which ultimately deletes the prerequisite backend modules. Add a build:backend step after setup run in order to fix this.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2802 +/- ##
=======================================
Coverage 71.75% 71.75%
=======================================
Files 134 134
Lines 7314 7315 +1
Branches 1521 1521
=======================================
+ Hits 5248 5249 +1
Misses 1938 1938
Partials 128 128 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
seems like my recent PR caused a merge conflict because i removed a deprecated dependency, my bad will take a look at this when I can, thanks for working on this! It looks great at a glance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Migrates the markbind-cli package’s runtime code from JavaScript to TypeScript, updating the repo build workflow to compile the CLI into packages/cli/dist and wiring the root build/dev scripts to consume the built CLI output.
Changes:
- Added TypeScript project configuration (including project references) and updated root build/dev scripts to use
tsc --build. - Converted CLI implementation files to
.ts, added local typings for the patchedlive-server, and updated CLI package publishing metadata to shipdist/. - Updated CLI unit/functional tests and developer docs to run against the compiled CLI output.
Reviewed changes
Copilot reviewed 22 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig_base.json | Enables resolveJsonModule for JSON imports across TS projects. |
| tsconfig.json | Switches to project-references layout and blocks accidental non-build tsc runs. |
| packages/cli/tsconfig.json | Adds CLI TS build config targeting packages/cli/dist. |
| packages/cli/index.ts | Migrated CLI entrypoint to TS and ES imports. |
| packages/cli/package.json | Publishes built output (dist/), adds TS build scripts, updates deps/types. |
| packages/cli/src/cmd/build.ts | TS port of build command. |
| packages/cli/src/cmd/deploy.ts | TS port of deploy command and updated Site invocation. |
| packages/cli/src/cmd/init.ts | TS port of init command with error typing tweaks. |
| packages/cli/src/cmd/serve.ts | TS port of serve command and live reload integration. |
| packages/cli/src/util/cliUtil.ts | TS port of CLI utility helpers. |
| packages/cli/src/util/ipUtil.ts | TS port + explicit parameter/return typing. |
| packages/cli/src/util/logger.ts | Moves CLI logger module to TS + ESM-style imports/exports. |
| packages/cli/src/util/serveUtil.ts | TS port of serve utilities (watch handlers + middleware). |
| packages/cli/src/lib/live-server/index.d.ts | Adds local type definitions for patched live-server. |
| packages/cli/test/unit/ipUtil.test.js | Updates unit test imports to use compiled dist/ output. |
| packages/cli/test/unit/cliUtil.test.js | Updates unit test imports to use compiled dist/ output. |
| packages/cli/test/functional/test.js | Runs functional tests against compiled CLI entrypoint. |
| package.json | Uses built CLI entrypoint for docs build/deploy scripts; adds tsx dev dependency. |
| .gitignore | Keeps local live-server typings tracked; ignores dist/ output folders. |
| .eslintignore | Ignores generated CLI dist/**/*.js in linting. |
| docs/devGuide/development/workflow.md | Notes using bundled tsx for running TS directly. |
| docs/devGuide/development/settingUp.md | Updates setup steps to build backend before npm link. |
| docs/images/debugger/WebStorm_1.png | Adds/updates IDE debugging screenshot asset. |
Comments suppressed due to low confidence (2)
packages/cli/index.ts:10
- TypeScript JSON modules (via resolveJsonModule) provide a default export;
package.jsondoes not have a named exportversion, so this import will fail type-checking. Import the JSON as a default value (or require it) and read.versionfrom it.
packages/cli/src/cmd/serve.ts:147 Site.generate(baseUrl)treats its argument as a CLI override forbaseUrl(it re-reads site config using this value). Passing an empty string here will overridesite.json's baseUrl and can break links/mounting when the site config baseUrl is non-empty. To preserve previous behavior ofsite.generate()in JS, passundefinedinstead.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
serveUtil (mistakenly) has a ts-ignore left in. Remove & add extra type definition to live-server to fix.
testEmptyDirectory was not using the CLI_PATH variable like the other tests in the file. This caused the test to fail.
dd6c577 to
eaebcc8
Compare
All good, no problem at all! Maybe I'll take this chance to mention something I forgot to talk about in the original PR message. I'm thinking of removing the build/dev scripts in the packages/cli package.json, because the CLI files heavily require the usage of core files. If you build and run just the cli package, either: a) You haven't built the core package and you get tons of import errors Instead of worrying about this I think we should just delegate build management of both packages to the one dev/build script in the root. So we remove the individual package build scripts. LMK if this makes sense. (We can discuss in the session later too) |
|
Adding @MoshiMoshiMochi and @yihao03 as reviewers for exposure, take a look if you can! Esp in regard to dev experience with the updated workflow. |
|
crazy effort! will take a good look later this evening |
same will take a look when i can |
|
In regard to Copilot's comment here:
I was wondering why the build was working even though the docs explicitly said it was required, but I was too tired at that point to care :p. Well, turns out the flag is not required in all cases. Only when the projects cross-reference each other. Well, either way I decided to set up the composite flag as it's supposed to facilitate faster checks. In doing so, I encountered a pretty silly error that I want to share. You don't need to read this btw: This took me way too long to realizeSo, I set what. I'm tripping. I run a) I should be setting So I know the first is verifiably false - I try setting what?? Okay. Maybe I set the You're probably noticing a pattern by now. And if you somehow know what's going on by now you get a cookie - DM to claim. I follow this pattern for a while. Making a change in the config, making ANY change that I could possibly think of that might maybe probably not fix the issue. At one point of time, I realized something. At one point of time I stopped trying to push, and instead I tested it by running "npm run clean" and then "npm run test". It was super flaky - first runs would pass then subsequent ones would fail. After a while of doing the debugging equivalent of running around like a headless chicken, I made a connection. What does the pre-push hook and the The janky (let's call it what it is) clean script looks for generated TypeScript files and deletes them, particularly, these:['.d.ts', '.d.ts.map', '.js', '.js.map']. By setting composite to true, TypeScript handles things a little differently and adds a lil extra. In each of the subprojects, TypeScript now stored a tsconfig.tsbuildinfo file. It reportedly stores "incremental compilation information" - I didn't pay much attention to it, and it wasn't even mentioned in the TypeScript Project References docs. It seems that even if the other build files are nuked, if TypeScript sees a tsconfig.tsbuildinfo file there, it assumes all is well. So this caused the So the very, very, very, very, very, very, very, very, very, very, very, simple solution to this very, very, very, very, very, very, very, very, annoying issue, is to simply delete that file. Thank you for reading. |
What is the purpose of this pull request?
Overview of changes:
Bear with me
Partially completes #2754
Migrate functional code in CLI package to typescript
index.jsand all util files converted to typescriptlive-serverpatch is still JavaScript - added type definitions for now.cli/dist/Package changes
cli/package.jsonto properly prepare and publish themarkbind-clipackage.package.jsonto use the built CLI module incli/dist/Developer-facing changes
cli/package.jsonbuild/devscripts are functionally the same as a developer)tsxto directly run filesAnything you'd like to highlight/discuss:
The migrated code is TypeScript, but it's not good TypeScript. There's liberal use of
any, so it could definitely be improved. I think we could do refactoring as a separate PR, as I didn't want to clutter this PR any further. LMK, though.Suggested things to refactor:
Refactor core
Siteconstructor. The CLI was using the constructor without properly specifying all the arguments - create an overloaded constructor for this?TypeScript 4.4 set the error type of catch variable to unknown. This means that to extract the error message, type narrowing was required. This introduces an extra branch and decision - what to log when the type of error is unknown? Currently, it simply logs "unknown error". Suggest: extract a variable/function that can be re-used in these cases, instead of hard-coding a message.
Create an interface/type that can be used for the
optionparameter of thecommanderpackage callbacksGenerally improve logger re-use in the
cmd/files - a lot of code duplication.Create an interface/type that can be used for the
ServerConfigobject inserve.tsDecrease the high level of coupling between the CLI and Core package
I chose
tsxoverts-nodesince tsx seems more actively-maintained (the last release forts-nodewas 3 years ago). Furthermore,tsxseems generally faster, and I had a good experience usingtsxwith Webstorm with zero configuration or issues. If there's another option worth considering, feel free to raise it up.In terms of documentation, I did not add an entry on how to configure VSCode with
tsxto run TypeScript code directly with, as I don't use VSC. If you think it's valuable, please do add it in!Testing instructions:
Test out that
npm linkworks with the new, TypeScript version of markbind-cli.For your convenience (adapt as necessary):
git fetch upstream pull/2802/head:arjunscoolprgit checkout arjunscoolprnpm uninstall -g markbind-clinpm run setupnpm run build:backendpackages/cliand link:npm linkTest out that
markbind-cliworks by running any command. Runnpm run cleanin the root directory. Check if the markbind-cli works. Now, build the project again and check if the markbind-cli works.I checked this with @MoshiMoshiMochi on Windows, and it looks to work as expected. I'm having some issues on Linux (odd permission issues - I could have just borked my setup so YMMV), so I would like to see if it works on other devices. Thanks in advance.
Additionally, I would appreciate a sanity check on the updated root
package.json. I updated the path of the CLI source, so everything should be working fine. I tested running the deploy scripts - it errored out after building the site due to the missing tokens so I think it works fine.Proposed commit message: (wrap lines at 72 characters)
Migrate functional code in CLI package to TypeScript
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):