Skip to content

Conversation

@Harjun751
Copy link
Contributor

@Harjun751 Harjun751 commented Jan 25, 2026

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Bear with me

Partially completes #2754

  • Migrate functional code in CLI package to typescript

    • index.js and all util files converted to typescript
    • live-server patch is still JavaScript - added type definitions for now.
    • All test files are still JavaScript
    • Add typescript configs in the cli package - generated files go to cli/dist/
  • Package changes

    • Modified cli/package.json to properly prepare and publish the markbind-cli package.
    • Modified root package.json to use the built CLI module in cli/dist/
    • Updated selected packages to get built-in type definitions
    • Installed some type definitions
  • Developer-facing changes

    • Added a build/dev script in cli/package.json
    • Updated root build/dev scripts to utilize Project References (running the build/dev scripts are functionally the same as a developer)
    • Added tsx as a dev dependency - run TypeScript files directly using the bundled runner
    • Update dev docs on modified setup instructions & on how to use tsx to directly run files

Anything 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:
  1. Refactor core Site constructor. The CLI was using the constructor without properly specifying all the arguments - create an overloaded constructor for this?

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

  3. Create an interface/type that can be used for the option parameter of the commander package callbacks

  4. Generally improve logger re-use in the cmd/ files - a lot of code duplication.

  5. Create an interface/type that can be used for the ServerConfig object in serve.ts

  6. Decrease the high level of coupling between the CLI and Core package

I chose tsx over ts-node since tsx seems more actively-maintained (the last release for ts-node was 3 years ago). Furthermore, tsx seems generally faster, and I had a good experience using tsx with 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 tsx to run TypeScript code directly with, as I don't use VSC. If you think it's valuable, please do add it in!

Testing instructions:

⚠️⚠️⚠️ HELP ME OUT HERE ⚠️⚠️⚠️

Test out that npm link works with the new, TypeScript version of markbind-cli.
For your convenience (adapt as necessary):

  1. Clone this PR: git fetch upstream pull/2802/head:arjunscoolpr
  2. Check out: git checkout arjunscoolpr
  3. Uninstall markbind-cli if present: npm uninstall -g markbind-cli
  4. Install updated project dependencies: npm run setup
  5. Build the project in the root directory: npm run build:backend
  6. Navigate to packages/cli and link: npm link

Test out that markbind-cli works by running any command. Run npm run clean in 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: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

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

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

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
Copy link

codecov bot commented Jan 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.75%. Comparing base (8a8a0c2) to head (ae2a80b).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gerteck
Copy link
Member

gerteck commented Jan 25, 2026

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

Copy link

Copilot AI left a 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 patched live-server, and updated CLI package publishing metadata to ship dist/.
  • 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.json does not have a named export version, so this import will fail type-checking. Import the JSON as a default value (or require it) and read .version from it.
    packages/cli/src/cmd/serve.ts:147
  • Site.generate(baseUrl) treats its argument as a CLI override for baseUrl (it re-reads site config using this value). Passing an empty string here will override site.json's baseUrl and can break links/mounting when the site config baseUrl is non-empty. To preserve previous behavior of site.generate() in JS, pass undefined instead.

💡 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.
@Harjun751 Harjun751 force-pushed the cli-typescript-migration branch from dd6c577 to eaebcc8 Compare January 26, 2026 02:10
@Harjun751
Copy link
Contributor Author

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

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
b) You have built the core package and it's outdated, so expected functionality is borked
c) You have built the core package and it's up-to-date so all works as expected

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)

@Harjun751
Copy link
Contributor Author

Adding @MoshiMoshiMochi and @yihao03 as reviewers for exposure, take a look if you can! Esp in regard to dev experience with the updated workflow.

@yihao03
Copy link

yihao03 commented Jan 26, 2026

crazy effort! will take a good look later this evening

@gerteck
Copy link
Member

gerteck commented Jan 27, 2026

will take a good look later this evening

same will take a look when i can

@Harjun751
Copy link
Contributor Author

Harjun751 commented Jan 27, 2026

In regard to Copilot's comment here:

This tsconfig is referenced by the root tsc --build graph, but it doesn't set compilerOptions.composite: true. Without composite, the TypeScript build mode will reject this project reference (TS6306). Add composite: true (and consider incremental/tsBuildInfoFile if desired) under compilerOptions.

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 realize

So, I set composite: true on the core package as that package was the one being referenced. I run tsc --build in the root directory, and yay. The project builds and I'm good to go. I make a commit and attempt to push. The pre-push hook runs and it fails.

what.

I'm tripping. I run npm run tests and it's failing too. What. I bring this up with AI, and it screams at me that:

a) I should be setting composite: true on the CLI package
b) What the hell are you doing, you're supposed to set rootDir in the package.json of core
c) Are you literally stupid? All implementation files must be matched by an include pattern or listed in the files array. (Quoted from typescript project ref docs)

So I know the first is verifiably false - I try setting rootDir and what do you know? I run the tests and they work. Maybe deforestation is worth it. I go to push it this time - I got it on lock. The pre-push hook fails again.

what??

Okay. Maybe I set the rootDir wrong - TypeScript docs say "set the rootDir to the common root of all project folders". Okay, that would mean that rootDir has to be the markbind/ folder itself right? I think? Whatever. I set it to that. It works, I run npm run test, all tests pass with flying colours and all is good with the world.

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 clean script have in common? They both run the clean script. The pre-push hook does it at the start. The issue was in the clean script, but what?

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 core package to skip building and when it was the CLI package's turn, it made a fuss because the files were, in fact, not there.

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.

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.

3 participants