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
|
Alright, updated the setting up workflow to something that's more sane. So @gerteck - let me know if there's any other issues. If all good I'll then focus on making a cleaned-up PR. No rush at all and thanks. |
gerteck
left a comment
gerteck
left a comment
There was a problem hiding this comment.
Thanks for working on this @Harjun751
Preliminary glance at this seems okay! You can go ahead to work on another PR, but I suspect that it will still have some changes here and there as well.
Additionally, could you do some investigation into
I wonder if it is relevant? Or was it some bug that has been patched, or why it was originally not suitable.
| "exclude": ["**/node_modules", "**/*.test.ts"], | ||
| // Prevent just `tsc` from building files - always use `tsc --build` | ||
| "files": [], | ||
| "references": [ |
There was a problem hiding this comment.
oh, does this build them in correct dependency order?
There was a problem hiding this comment.
Yep, using the --build option resolves the referenced dependencies in the proper order.
| function printHeader() { | ||
| logger.logo(); | ||
| logger.log(` v${CLI_VERSION}`); | ||
| return ''; |
There was a problem hiding this comment.
It was a very band-aid fix since commander actually expects a string return value for the addHelpText function call. That is, we weren't really using it properly haha.
I could extract the logo print from the logger, but I felt it'd probably be better to address this in a PR specifically targeted to refactor/improve this.
There was a problem hiding this comment.
got it! in that case maybe open an issue to track it jsut to rmb
Alright, I'll do some proper investigation. But on a preliminary check, if this issue still exists we could reasonably circumvent this by removing the |
|
TL;DR Upon investigation,
Dev team concerns
|
istextorbinary developer dependency uses an old version without typescript type definitions. As part of migration of tests to typescript, the files requiring istextorbinary now need type definitions. Let's update istextorbinary to the latest version. No breaking changes were introduced that affected the usage of istextorbinary in the code.
To facilitate TypeScript migration, update `diff` package to get in-built type declarations. No breaking changes related to current usage introduced.
test.ts uses the exported testSites to iterate and index as a dict to reference built files for checking. Add an interface to explictly show that the object may be indexed via a string to obtain files, allowing for stricter type checks.
With migrated test files, eslint does not detect that functional test files are tests, resulting in raising "no-extraneous-dependencies" error. This error suggests that dependencies used in the actual application and not test harnesses/files should be in the main dependencies section. However, since these are test files, the dependencies should rightfully be in `devDependencies`. Hence, let's add an rule exclusion for import/no-extraneous-dependencies for the functional tests.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 209 out of 272 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
packages/cli/src/util/logger.ts:27
- The winston logger configuration is removing deprecated options
humanReadableUnhandledExceptionandshowLevelfromDailyRotateFiletransport. However, no migration notes or warnings are provided. If these options were being used for specific logging behavior, their removal could affect log output format or exception handling without proper documentation of the changes.
packages/cli/src/util/logger.ts:53 - The logger is now using named exports from '@markbind/core/src/utils/logger', but also defining local
logandlogoexports. Thelogfunction is a simple wrapper aroundconsole.logthat adds a type annotation for the message parameter. However, the type is too restrictive -console.logaccepts any type, not just strings. This could cause type errors when trying to log non-string values. Consider changing the type toanyor accepting multiple arguments likeconsole.logdoes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| new Site(rootFolder, outputFolder, '', undefined, options.siteConfig, | ||
| false, false, () => {}) |
There was a problem hiding this comment.
The Site constructor calls in deploy.ts and build.ts are passing additional parameters that weren't in the original JavaScript version. Specifically, they're passing empty strings and callback functions as positional parameters. This appears to be a workaround for TypeScript's strict type checking, but it's unclear if these parameters are actually required or if the constructor signature has changed. This should be verified against the actual Site constructor signature to ensure compatibility.
| new Site(rootFolder, outputFolder, '', undefined, options.siteConfig, | |
| false, false, () => {}) | |
| new Site(rootFolder, outputFolder, options.siteConfig) |
There was a problem hiding this comment.
I think this can be addressed in the refactor (will open an issue once the migration is merged).
I was preserving the way it was done pre-migration.
|
|
||
| # Compiled files from TypeScript | ||
| *.js.map | ||
| *.d.ts |
There was a problem hiding this comment.
hope that this does not affect anything
just to point it out since the commit addes index.d.ts for live-server
| # --- packages/core end --- | ||
|
|
||
| # Manual type definitions need to be included | ||
| !packages/cli/src/lib/live-server/index.d.ts |
There was a problem hiding this comment.
oh this was added
actually if everything gets compiled to dist we could overhaul the gitignore and simplify right
|
Moved to #2830 |
3 participants
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):