Skip to content

Conversation

@jfmengels
Copy link
Owner

The purpose of this PR is to use TypeScript to type-check the JS code of the CLI, to feel more at ease when changing that part of the codebase, which todays feels quite scary to be honest.

It's apparently quite do-able to have TS understand type annotations through JSDoc comments, so I think I want to try that instead of converting files to .ts. If that works out well, then we can skip the transpilation phase, which will keep it pretty smooth to run the CLI in development mode (no need to wait for TS to compile the project).

We can potentially also use this to publish .d.ts file, in case someone wants to depend on the JS API of the tool (like elm-tooling/elm-language-server#942). I imagine this would help, but I could be wrong.

Status of the PR

I'm finding a bunch of issues that I add to the main branch, but this PR is not ready yet.
First of all, I have a large number of problems that TS reports that seem like configuration errors.
Second, I still need to add TS to the test script so that we're made aware of TS errors.

In a separate PR, we could then remove Flow.

I'd appreciate any help with this work and/or with adding type annotations to the JS codebase (better done in separate commits).

@socket-security
Copy link

socket-security bot commented Apr 22, 2023

New dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

⚠️ Please accept the latest app permissions to ensure bot commands work properly. Accept the new permissions here.

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
[email protected] None +0 typescript-bot
[email protected] None +4 lydell
[email protected] eval, filesystem +0 prettier-bot

🚮 Removed packages: [email protected], [email protected], [email protected]

tsconfig.json Outdated
Comment on lines 3 to 9
"allowJs": true,
"checkJs": true,
"resolveJsonModule": true,
"skipLibCheck": true,
"target": "es2015",
"moduleResolution": "node",
"noEmit": true
Copy link
Contributor

Choose a reason for hiding this comment

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

Just in case you didn’t know: At some point you probably want to make the typechecking more comprehensive, especially adding "strict": true. More candidates:

    "strict": true,
    "noFallthroughCasesInSwitch": true,
    "noImplicitReturns": true,
    "useUnknownInCatchVariables": true

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks! I knew I didn't check for much (because I first want to get rid of the configuration-related errors), but I didn't know exactly which things I needed to be enable. This saves me some times 😊
If you have more tips and candidates, I'll take them. I think there was one about implicit any for instance, which I'll try to find at some point.

Copy link
Contributor

Choose a reason for hiding this comment

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

strict is a shortcut for enabling a bunch of other options, including noImplicityAny. I couldn’t find the exact list of options in TypeScript’s documentation, but I found this in a blog post:

    noImplicitAny
    noImplicitThis
    alwaysStrict
    strictBindCallApply
    strictNullChecks
    strictFunctionTypes
    strictPropertyInitialization

I went through the elm-watch tsconfig.json, and the last strictness related one seems to be:

    "noUncheckedIndexedAccess": true,

But when adding TypeScript to an already existing code base it might make sense to enable one thing at a time so you don’t drown in errors, before you have even learned how to deal with many of them.

The final level is to bring in typscript-eslint. Then you get close to Elm level type system confidence.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Perfect, thanks :)

@jfmengels
Copy link
Owner Author

I found solutions to some of these last issues, and ts-ignored the rest :(

@lydell
Copy link
Contributor

lydell commented Apr 29, 2023

@jfmengels Try adding "esModuleInterop": true, to tsconfig.json. I think that should do it for both elm-tooling and got.

@lydell
Copy link
Contributor

lydell commented Apr 29, 2023

Nope, I was wrong about "esModuleInterop": true. It has helped me before with dependencies in other projects though!

For me, this works:

const { default: elmToolingCli } = require("elm-tooling");
const { default: getExecutable } = require("elm-tooling/getExecutable");

EDIT: It typechecks. But does it work at runtime? Need to check. EDIT2: Nope. Why is TypeScript so hard to get right with dependencies…

@jfmengels
Copy link
Owner Author

EDIT: It typechecks. But does it work at runtime? Need to check. EDIT2: Nope. Why is TypeScript so hard to get right with dependencies…

Yes, I went through the same cycle you just went through 😅

@lydell
Copy link
Contributor

lydell commented Apr 29, 2023

Oh well, I must have misunderstood something and made some mistake in elm-tooling. I’ve only tested it with pure JS (both require and import) and .ts TypeScript with import before.

So maybe just go with @ts-ignore (or @ts-expect-error) for now. Maybe the PR getting rid of elm-tooling gets finished soon anyway…

@lydell
Copy link
Contributor

lydell commented Apr 29, 2023

Found a workaround if you really want typechecking for those calls:

const elmToolingCliImport = require("elm-tooling");
const getExecutableImport = require("elm-tooling/getExecutable");

const elmToolingCli = /** @type {typeof elmToolingCliImport["default"]} */ (
  /** @type {unknown} */ (elmToolingCliImport)
);

const getExecutable = /** @type {typeof getExecutableImport["default"]} */ (
  /** @type {unknown} */ (getExecutableImport)
);

@jfmengels
Copy link
Owner Author

Thanks, I'll keep it as is for now I think, but happy to know that your comment will be here if I change my mind 😄

@jfmengels jfmengels marked this pull request as ready for review April 29, 2023 22:02
@jfmengels jfmengels merged commit 2c31f02 into master Apr 29, 2023
@jfmengels jfmengels deleted the add-typescript branch April 29, 2023 22:07
@lishaduck lishaduck mentioned this pull request Oct 15, 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.

3 participants