Conversation
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.
To facilitate TypeScript migration, update `diff` package to get in-built type declarations. No breaking changes related to current usage introduced.
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 configure adapted tests and files to run properly, scripts need to be updated. Additionally, project references should be enabled to ensure that subpackages referring to each other are built in the correct order and correctly. Let's * Update root package.json to use `tsc --build` to build the backend * Update scripts that require CLI to use built output (.js files) * Update cli jest.config.ts to use ts-jest to be able to run test files with jest directly * Update root tsconfig to point to cli/ and core/ packages * Update core/ tsconfig to set `composite` to true to utilize incremental builds * Add cli/ tsconfig with references to core/ package, along with basic config * Update packages in package-lock
Due to update TypeScript configurations, update the developer docs to remain up-to-date on setting up the project and developing.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2830 +/- ##
==========================================
- Coverage 72.20% 72.20% -0.01%
==========================================
Files 134 134
Lines 7454 7461 +7
Branches 1528 1566 +38
==========================================
+ Hits 5382 5387 +5
- Misses 2026 2028 +2
Partials 46 46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
LGTM, good job on this
Note to self:
--buildfortscis for project references, which builds each package in dependency order.
Other stuff
-
CLI snapshot test fails on my local after checking out to this branch. (EDIT: Due to
.DS_Storedue to dev on MacOS, unrelated to this PR. Tests pass on CI.) -
I got a permission denied as in, had to run chmod to add execute perms. i wonder if it is just a local dev env thing? Have not faced this before.
Generated js file does not have executable flag enabled.
Edit: The markbind CLI command fails with "permission denied" because the generated packages/cli/dist/index.js
file loses its executable flag during the build process. tsc does not preserve permissions from source files.
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:
Above running normal tests, you can check if your IDE/setup can correctly run individual test cases/suites in the cli/test/unit directory, since they use TypeScript now.
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):