add type checking

[irahopkinson]

• fixes Add tsc typechecking to pre-commit to catch Typescript errors #1085
• include core, extensions, platform-bible-utils, platform-bible-react
• fix existing type checking errors
• add typecheck to pre-commit

use noImplicitReturns instead of consistent-return

---

This change is 

[jolierabideau]

Thanks Ira!

Reviewed 32 of 32 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)

---

extensions/lib/update-from-templates.ts line 69 at r1 (raw file):

        if (
          includes(
            errorMessage,

You could also get the error message with getErrorMessage from platform-bible-utils

[tjcouch-sil]

Thanks for the hard work on this! Hopefully this will make it harder to break things :)

Once all of this is said and done, would you mind making the appropriate changes in the templates too please? Like updating the git utils code and such.

Reviewed 29 of 32 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @irahopkinson and @jolierabideau)

---

tsconfig.json line 51 at r1 (raw file):

    "lib/platform-bible-react",
    "lib/platform-bible-utils",
    "lib/papi-dts"

Why remove this? Core probably shouldn't have access to papi-dts because papi-dts is generated off of core. Changing types might cause there to be two different definitions of a type until you re-run build:types, which would probably get really annoying. I'm also surprised it doesn't show errors that there are two definitions of types in general to be honest.

If the intention was to add type checking for papi.d.ts (which I think is a good idea as we sometimes get errors in there, I think), I suggest potentially adding another typecheck:papi-dts or something

---

tsconfig.lint.json line 4 at r1 (raw file):

  "extends": "./tsconfig",
  "compilerOptions": {
    "skipLibCheck": true

Oh maybe this is the reason there aren't errors about duplicate definitions of types. Why add this?

---

extensions/tsconfig.json line 26 at r1 (raw file):

    "typeRoots": [
      // Include default type declarations
      "../node_modules/@types",

This is not the default path for type declarations anymore to my understanding. I suggest adding another line right below this one or changing the comment on this line.

---

extensions/lib/add-remotes.ts line 14 at r1 (raw file):

  let exitCode = 0;

  // Helper function to handle remote addition

What happened to all this? Did typescript report errors on this or something?

---

extensions/lib/git.util.ts line 93 at r1 (raw file):

    if (!quiet && result.stderr) console.log(result.stderr);
    return result;
  } catch (error: unknown) {

Why add unknown here? Is TS requiring that or something? I thought it implicitly understood it was unknown.

---

extensions/lib/git.util.ts line 104 at r1 (raw file):

      );
    } else {
      throw new Error('An unknown error occurred while executing git command');

Could you add the error in at the end of the string in hopes that it will give some more specific info to the user?

I'm particularly concerned about these git-related errors because not all of them will be easily repeatable since they're interacting with stuff on file system, in git state, and online. I'd like to make absolutely certain we can give whatever error information is available to us.

---

extensions/lib/git.util.ts line 146 at r1 (raw file):

      console.error(`Error on git fetch on ${SINGLE_TEMPLATE_NAME}: ${error.message}`);
    } else {
      console.error(`An unknown error occurred while fetching from ${SINGLE_TEMPLATE_NAME}`);

Why make this change? Isn't it good to print the error in all contexts? I think the difference between e.toString() and e.message is just that the message doesn't have "Error: " at the start, which I'd consider to be not worth all the trouble.

If you'd like to keep this change, could you please add the error in at the end of the string here in hopes that it will give some more specific information to the user?

---

extensions/lib/update-from-templates.ts line 69 at r1 (raw file):

Previously, jolierabideau wrote…

You could also get the error message with getErrorMessage from platform-bible-utils

I'm still not understanding the motivation to change the code here. Would like to understand better.

(Also note platform-bible-utils is not available yet in all contexts in which this code will be running. This code is from the multi template)

---

extensions/lib/update-from-templates.ts line 87 at r1 (raw file):

      } else {
        console.error(
          `An unknown error occurred while pulling from ${SINGLE_TEMPLATE_NAME} to ${ext.dirName}`,

Same comment as in other places; would like to have the error printed out if these error reworks are not reverted.

---

extensions/src/hello-world/src/web-views/hello-world.web-view.tsx line 264 at r1 (raw file):

        <Checkbox labelText="Test Me" />
        <Switch /> {/* no label available */}
        <ComboBox title="Test Me" options={['option 1', 'option 2']} />

This prop was not available, right? This seems like a significant oversight worth filing a bug on if I am understanding correctly.

---

lib/platform-bible-react/tsconfig.json line 5 at r1 (raw file):

  "compilerOptions": {
    "baseUrl": "./src",
    "rootDir": "./src",

What is the effect of removing this line here and in utils? Is it redundant? I would have guessed this change would make rootDir stay up at repo top level since we're extending that tsconfig. If that would be the case, I think all the built files would be two extra folders deep, which seems strange.

---

lib/platform-bible-utils/package.json line 5 at r1 (raw file):

  "version": "0.0.1",
  "type": "module",
  "description": "Utilities to use in Platform.",

I think we decided we would do all of these all at once at some point after we get the final green light from UX that this is the final name so we wouldn't miss any, right? If so, I'd recommend reverting for now unless you heard final confirmation that this is the final name. It's just so hard to find "Platform" only when it refers to our software that it would be good to wait and make sure.

---

src/extension-host/services/extension.service.ts line 767 at r1 (raw file):

  // "Packaged" extensions are all the running extensions that aren't "enabled".
  // `undefined` items are filtered out so can assert here.
  // eslint-disable-next-line no-type-assertion/no-type-assertion

Do you know why this is needed all of a sudden? No error shows up on my computer here. active shows up as not undefined and packaged as ExtensionIdentifier[], so I wouldn't think TS would need something here.

Oh maybe we need to update typescript! I believe this maybe would have been necessary before TypeScript 5.5 made it possible. VS Code bundles in TS 5.5 already, but looks like our repos use 5.4.5 still.

[irahopkinson]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jolierabideau and @tjcouch-sil)

---

tsconfig.json line 51 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Why remove this? Core probably shouldn't have access to papi-dts because papi-dts is generated off of core. Changing types might cause there to be two different definitions of a type until you re-run build:types, which would probably get really annoying. I'm also surprised it doesn't show errors that there are two definitions of types in general to be honest.

If the intention was to add type checking for papi.d.ts (which I think is a good idea as we sometimes get errors in there, I think), I suggest potentially adding another typecheck:papi-dts or something

I think I was just diagnosing and didn't put it back. BTW, having lib/papi-dts in this exclude section doesn't actually stop it from being included in core if it's imported in core code - importing overrides exclude. However, it's a good idea to type check it. Done.

---

tsconfig.lint.json line 4 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Oh maybe this is the reason there aren't errors about duplicate definitions of types. Why add this?

If you don't, it checks all the files in libraries including in node_modules and there is always some problems there that we can't easily fix.

---

extensions/tsconfig.json line 26 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This is not the default path for type declarations anymore to my understanding. I suggest adding another line right below this one or changing the comment on this line.

Now that this repo is an npm workspace the only valid node_modules folder is at the root. When you add types from DefinitelyTyped this is the folder they go in so that seems to me like the default type location. What am I missing?

---

extensions/lib/add-remotes.ts line 14 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

What happened to all this? Did typescript report errors on this or something?

Yes, the previous code was causing type checking errors.

---

extensions/lib/git.util.ts line 93 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Why add unknown here? Is TS requiring that or something? I thought it implicitly understood it was unknown.

Just making that explicit so we know that we need to check the type of error before using it.

---

extensions/lib/git.util.ts line 104 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Could you add the error in at the end of the string in hopes that it will give some more specific info to the user?

I'm particularly concerned about these git-related errors because not all of them will be easily repeatable since they're interacting with stuff on file system, in git state, and online. I'd like to make absolutely certain we can give whatever error information is available to us.

I specifically left it out so we don't throw another exception while we are dealing with the first one. If we ever get one of these a diagnostic step could be to temporarily add it in to see if it reveals anything.

---

extensions/lib/git.util.ts line 146 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Why make this change? Isn't it good to print the error in all contexts? I think the difference between e.toString() and e.message is just that the message doesn't have "Error: " at the start, which I'd consider to be not worth all the trouble.

If you'd like to keep this change, could you please add the error in at the end of the string here in hopes that it will give some more specific information to the user?

Again I left it out because we don't know the type so we are limited in what we can do and we don't want to hide this error by causing another exception.

---

extensions/lib/update-from-templates.ts line 69 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I'm still not understanding the motivation to change the code here. Would like to understand better.

(Also note platform-bible-utils is not available yet in all contexts in which this code will be running. This code is from the multi template)

I think I have explained in the previous comments.

---

extensions/lib/update-from-templates.ts line 87 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Same comment as in other places; would like to have the error printed out if these error reworks are not reverted.

See other comments.

---

extensions/src/hello-world/src/web-views/hello-world.web-view.tsx line 264 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This prop was not available, right? This seems like a significant oversight worth filing a bug on if I am understanding correctly.

Yes, the prop doesn't exist. ComboBox is a deprecated MUI component. If you think it's worth fixing then please file an issue.

---

lib/platform-bible-react/tsconfig.json line 5 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

What is the effect of removing this line here and in utils? Is it redundant? I would have guessed this change would make rootDir stay up at repo top level since we're extending that tsconfig. If that would be the case, I think all the built files would be two extra folders deep, which seems strange.

rootDir is "The longest common path of all non-declaration input files.". Here it is effectively "./" which is what we need for linting and type checking. Also note from the docs "Importantly, rootDir does not affect which files become part of the compilation."

---

lib/platform-bible-utils/package.json line 5 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think we decided we would do all of these all at once at some point after we get the final green light from UX that this is the final name so we wouldn't miss any, right? If so, I'd recommend reverting for now unless you heard final confirmation that this is the final name. It's just so hard to find "Platform" only when it refers to our software that it would be good to wait and make sure.

Good call. Reverted here and PBR.

---

src/extension-host/services/extension.service.ts line 767 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Do you know why this is needed all of a sudden? No error shows up on my computer here. active shows up as not undefined and packaged as ExtensionIdentifier[], so I wouldn't think TS would need something here.

Oh maybe we need to update typescript! I believe this maybe would have been necessary before TypeScript 5.5 made it possible. VS Code bundles in TS 5.5 already, but looks like our repos use 5.4.5 still.

In VS Code, you can set it to use TypeScript from the repo, then checkout this branch and try removing the comment. Or just check out the branch and run the relevant typecheck script. It errors for me. I didn't investigate why it wasn't causing a problem before. You're probably right about a later version of TS but that's out of scope for this PR.

[tjcouch-sil]

Reviewed 3 of 32 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @irahopkinson and @jolierabideau)

---

tsconfig.lint.json line 4 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

If you don't, it checks all the files in libraries including in node_modules and there is always some problems there that we can't easily fix.

Hmm can you exclude node_modules? I'm thinking it would probably be good to check our code in context of the .d.ts files that we create.

(Marking this as open instead of blocking)

---

extensions/tsconfig.json line 26 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Now that this repo is an npm workspace the only valid node_modules folder is at the root. When you add types from DefinitelyTyped this is the folder they go in so that seems to me like the default type location. What am I missing?

Ah I was under the impression that they recently changed typeRoots so it doesn't search up the path anymore but just searches node_modules/@types and that's it. Sorry.

---

extensions/lib/git.util.ts line 104 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I specifically left it out so we don't throw another exception while we are dealing with the first one. If we ever get one of these a diagnostic step could be to temporarily add it in to see if it reveals anything.

All I'm suggesting to do in this one is change this one line to the following:

throw new Error(`An unknown error occurred while executing git command. ${error}`);

Is there any reasonable context in which putting a variable in a string template literal will cause an error to occur? My understanding is that we very commonly all over our code just string template literal the error into the message to give extra information about the error that gets thrown. If someone were to throw an object whose toString throws an error, that would cause this to fail. But I'd suggest that's not worth worrying about.

I would probably suggest just reverting in the other spots where a similar change to this occurred since they already pretty much had this same thing and are just more complex with the new code. But an alternate option would be just to string template literal the error into those as well like in my suggested code here.

---

extensions/src/hello-world/src/web-views/hello-world.web-view.tsx line 264 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Yes, the prop doesn't exist. ComboBox is a deprecated MUI component. If you think it's worth fixing then please file an issue.

#1092 :)

[irahopkinson]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @jolierabideau and @tjcouch-sil)

---

tsconfig.lint.json line 4 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Hmm can you exclude node_modules? I'm thinking it would probably be good to check our code in context of the .d.ts files that we create.

(Marking this as open instead of blocking)

Yeah, I didn't like adding this but I reasoned that some type checking is better than none. We might be able to improve this by using both typeRoots and types. I'll investigate a bit more...

---

extensions/tsconfig.json line 26 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Ah I was under the impression that they recently changed typeRoots so it doesn't search up the path anymore but just searches node_modules/@types and that's it. Sorry.

No problem. If you don't specify typeRoots https://www.typescriptlang.org/tsconfig/#typeRoots suggests it searches up the tree. Prior to looking at this again I was under the same impression that they changed this. Maybe they changed something related to this - I don't actually remember.

---

extensions/lib/git.util.ts line 104 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

All I'm suggesting to do in this one is change this one line to the following:

throw new Error(`An unknown error occurred while executing git command. ${error}`);

Is there any reasonable context in which putting a variable in a string template literal will cause an error to occur? My understanding is that we very commonly all over our code just string template literal the error into the message to give extra information about the error that gets thrown. If someone were to throw an object whose toString throws an error, that would cause this to fail. But I'd suggest that's not worth worrying about.

I would probably suggest just reverting in the other spots where a similar change to this occurred since they already pretty much had this same thing and are just more complex with the new code. But an alternate option would be just to string template literal the error into those as well like in my suggested code here.

I understand what you are suggesting. If we did that and error doesn't have a toString()function then this would throw another exception which would hide the original exception. Are you ok with that?

[irahopkinson]

Reviewable status: 23 of 49 files reviewed, 4 unresolved discussions (waiting on @jolierabideau and @tjcouch-sil)

---

tsconfig.lint.json line 4 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Yeah, I didn't like adding this but I reasoned that some type checking is better than none. We might be able to improve this by using both typeRoots and types. I'll investigate a bit more...

This is looking better now. I couldn't remove it from /extensions - the biggest issue was all the imports of '<someFile>?inline'. I have one more idea to try but it seems a long shot so if that works I'll do a follow-up PR.

[irahopkinson]

Reviewable status: 23 of 49 files reviewed, 4 unresolved discussions (waiting on @jolierabideau and @tjcouch-sil)

---

tsconfig.lint.json line 4 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This is looking better now. I couldn't remove it from /extensions - the biggest issue was all the imports of '<someFile>?inline'. I have one more idea to try but it seems a long shot so if that works I'll do a follow-up PR.

I have it working for extensions now. And as a bonus, I have a clear reference to all the types that need fixing in the BNF repo (I think I have a way to fix that now - I just need time to work on it).

[irahopkinson]

I'll create PRs for the 2 templates and once they are merged I'll redo this PR including those updates (now that I think we have things worked out).

Reviewable status: 20 of 49 files reviewed, 4 unresolved discussions (waiting on @jolierabideau and @tjcouch-sil)

---

extensions/lib/add-remotes.ts line 14 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Yes, the previous code was causing type checking errors.

I turns out with all the changes we are now only type checking src files so I've reverted this. I will take some of the good changes and apply them to the template.

---

extensions/lib/git.util.ts line 104 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I understand what you are suggesting. If we did that and error doesn't have a toString()function then this would throw another exception which would hide the original exception. Are you ok with that?

I turns out with all the changes we are now only type checking src files so I've reverted this. I will take some of the good changes and apply them to the template.

---

extensions/lib/git.util.ts line 146 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Again I left it out because we don't know the type so we are limited in what we can do and we don't want to hide this error by causing another exception.

I turns out with all the changes we are now only type checking src files so I've reverted this. I will take some of the good changes and apply them to the template.

---

extensions/lib/update-from-templates.ts line 69 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I think I have explained in the previous comments.

I turns out with all the changes we are now only type checking src files so I've reverted this.

---

extensions/lib/update-from-templates.ts line 87 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

See other comments.

I turns out with all the changes we are now only type checking src files so I've reverted this.

[tjcouch-sil]

Reviewed 4 of 26 files at r3, 29 of 32 files at r6, all commit messages.
Reviewable status: 46 of 54 files reviewed, 1 unresolved discussion (waiting on @jolierabideau)

---

extensions/lib/git.util.ts line 104 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I turns out with all the changes we are now only type checking src files so I've reverted this. I will take some of the good changes and apply them to the template.

I suggest the likelihood that an object can't be string templated is extremely low and not worth worrying about.

---

src/renderer/global-this.model.ts line 77 at r6 (raw file):

  // @ts-expect-error ts(2300) we're declaring React here because it may not always be available in
  // all contexts
  var React: typeof ReactModule;

Why remove this? We included it so people know we have set up globalThis.React for if they want to use it for some reason. I see you made some other React type adjustments I didn't understand, so I figure there's probably something going on here I don't understand.

[jolierabideau]

Reviewed 5 of 26 files at r3, 3 of 32 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[irahopkinson]

Reviewable status: complete! all files reviewed, all discussions resolved

---

src/renderer/global-this.model.ts line 77 at r6 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Why remove this? We included it so people know we have set up globalThis.React for if they want to use it for some reason. I see you made some other React type adjustments I didn't understand, so I figure there's probably something going on here I don't understand.

I had to add src/@types/react.d.ts to overcome the problem described in that file. It's doing that through a global also so that it can override and then ignore the type checking error and this line was in conflict with that global. A brute-force fix for this would be to skip lib checks. Another option would be for me to add the TS ignore to node_modules/@types/react/index.d.ts:46 and patch the package. BTW first I had to remove the @ts-expect-error ts(2300) here as it is no longer erroring.

[tjcouch-sil]

Reviewed 5 of 26 files at r3, 3 of 32 files at r6, 5 of 5 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved