Skip to content
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

Refactor type-checking setup #1969

Merged
merged 11 commits into from Oct 4, 2023
Merged

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Aug 14, 2023

Pull Request

Problem

Type-checking of the JavaScript files using TypeScript was quite fragile and partially misconfigured:

  • @ts-check comments were not working to enable checks in JavaScript code in VSCode, as only works on first line
  • different settings used by npm run typescript-checkJS than in VSCode so confusing
  • npm run typescript-checkJS didn't work on Windows (glob not expanded)

Related: #1960 #1962

Solution

  • tsconfig.json with less strict settings covering mixed JavaScript and TypeScript
    • used by VSCode
  • tsconfig.ts.json
    • stricter settings
    • just the TypeScript files
    • used by
      • typecheck-ts run-script
      • eslint
      • ts-jest
    • typecheck-ts now included in test run-script for strict checks on the published TypeScript typings
  • tsconfig.js.json
    • just the JavaScript files
    • used by typecheck-js.json run-script
  • remove glob in run-script so works on Windows
  • remove @ts-check comments as JavaScript processing now turned on in tsconfig.json

Explicitly separates the TypeScript and JavaScript files for using with npm run-scripts et al, and run-script naming, following suggestions by @aweebit.

I also updated the lib and module and target settings to match suggested configuration from https://github.com/tsconfig/bases/blob/main/bases/node16.json

References:

ChangeLog

  • (dev) improve setup for type-checking of JavaScript files

@shadowspawn shadowspawn marked this pull request as draft August 14, 2023 11:36
@shadowspawn shadowspawn marked this pull request as ready for review August 15, 2023 09:04
tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 15, 2023
@aweebit
Copy link
Contributor

aweebit commented Aug 15, 2023

Please take a look at my vision for this refactor: shadowspawn/commander.js@feature/type-checking...aweebit:commander.js:feature/type-checking. Explanations are given in the commit descriptions and the comment in jsconfig.json.

You can simply merge my branch into yours if you like the changes.

@aweebit
Copy link
Contributor

aweebit commented Aug 15, 2023

@shadowspawn shadowspawn marked this pull request as draft August 16, 2023 09:15
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Aug 16, 2023

Moved to Draft while I have a look at ideas from #1969 (comment)

@shadowspawn shadowspawn marked this pull request as ready for review August 17, 2023 08:00
@shadowspawn
Copy link
Collaborator Author

FYI @aweebit : I was interested in your idea of having extra typings/tsconfig.json to get strict type checking in VSCode for that folder, but it is not supported by tsc and I am not certain it is explicitly supported by VSCode. The wording around tsconfig.json is about the file being in the root of the project, so I kept to an expected layout.

@aweebit
Copy link
Contributor

aweebit commented Aug 17, 2023

FYI @aweebit : I was interested in your idea of having extra typings/tsconfig.json to get strict type checking in VSCode for that folder, but it is not supported by tsc

That is why I named the main TSConfig file jsconfig.json instead of tsconfig.json: tsc should not be run without an explicitly specified --project, instead it should ideally be run from npm run typecheck with the right config files.

and I am not certain it is explicitly supported by VSCode.

Not sure what you mean by "explicitly" here, but it is supported. Try this on my branch:

echo 'export let myVar;' > demo.ts
cp demo.ts typings/demo.ts
code demo.ts # no error
code typings/demo.ts # error

@aweebit
Copy link
Contributor

aweebit commented Aug 17, 2023

By the way, I used project references to separate configs in #1975. Works really well.

@shadowspawn
Copy link
Collaborator Author

Not sure what you mean by "explicitly" here, but it is supported.

By "explicitly" I mean that it is an intended pattern and not just a happy accident it works. e.g. documented, or used in official examples.

By the way, I used project references to separate configs in #1975. Works really well.

Thanks. I did read about project references, but I did not feel it was a good fit.

"typescript-checkJS": "tsc --allowJS --checkJS index.js lib/*.js --noEmit",
"test-all": "npm run test && npm run lint && npm run typescript-checkJS && npm run test-esm"
"typecheck-ts": "tsd && tsc -p tsconfig.ts.json",
"typecheck-js": "tsc -p tsconfig.js.json",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about typecheck:ts instead of typecheck-ts?
We already use lint:typescript.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been following a pattern I saw and liked long ago, but it is arbitrary. I use the colon for sub-scripts which are the parts of a combo script. So lint:typescript is one part of lint.

"lint": "npm run lint:javascript && npm run lint:typescript",

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@shadowspawn shadowspawn merged commit 96c6c25 into tj:develop Oct 4, 2023
11 checks passed
@shadowspawn shadowspawn deleted the feature/type-checking branch October 4, 2023 07:37
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Oct 7, 2023
@jasmin2014

This comment was marked as off-topic.

@shadowspawn

This comment was marked as off-topic.

@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Feb 3, 2024
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.

None yet

4 participants