Ensure all scripts are checked, fix errors

[jakebailey]

While working on a new lint rule, I noticed that a lot of the scripts have errors when I open them in the editor for various reasons, be it improper imports, type errors, etc.

Fix everything up and ensure any .ts files are included in this project, so we see their errors at build time.

There are a couple of the scripts that are written in js directly, so have handwritten d.ts files for them. I don't like that, but I think there must be some external callers (I see VSTS mentioned) that rely on being able to run the project without being compiled, it seems, so I won't change the status quo there quite yet.

[DanielRosenwasser]

These are arguably more correct though, right?

[jakebailey]

Do you mean the old requires, or the new imports? (Some of the scripts already used imports; I have these changes also on my module transform branch too.)

[jakebailey]

(The output code is the same; with the imports, we generate const fs = require("fs") exactly)

[DanielRosenwasser]

I meant the old requires since those correspond to these being true CommonJS require calls anyway. It probably doesn't matter, so long as glob doesn't get called.

[jakebailey]

Ah, are you suggesting import glob from "glob"? I can do that, so long as we enable esModuleInterop. I opted to not change the tsconfig settings since import * seem to work.

[DanielRosenwasser]

Right so that's the thing - import/require allows us to not worry about interop at all because we get to just say "yeah, the thing Node actually does with CJS.

The alternative is to swap to ESM scripts which I am actually open to as well for dog-fooding purposes. But maybe do that as a separate PR.

[jakebailey]

Yes, that is actually the next step I wanted to take, but I wanted to get them to type check first.

[amcasey]

I'm missing something - why are we changing import styles?

[jakebailey]

When I open say, scripts/configureLanguageServiceBuild.ts, this is what I get:

Import assignment cannot be used when targeting ECMAScript modules. Consider using 'import * as ns from "mod"', 'import {a} from "mod"', 'import d from "mod"', or another module format instead.

Technically, this is because the files are not included in the scripts project, so are getting some unspecified config. Fixing tsconfig.json to include all of the scripts fixes that.

I can totally undo the import changes and just leave the *.ts change; I originally fixed the imports directly, then later figured out that half of the scripts were not included in the project (so had other errors). Since half of the tests already use the "normal" import style, like buildProtocol.ts, I just left them as rewritten to be consistent when I sent the PR. I didn't expect that the import style would be the contentious thing 😃

[andrewbranch]

I think as long as they type check and run, it doesn’t matter that much, but for my 2 cents, I think it’s strange to have ESM- and CJS-style imports in the same file, which is what we have before the change. I am totally on board with Daniel’s assessment that the requires are technically “more correct” but only if we also replace all the import { readFile } from "fs" with import fs = require("fs") too. So I think the proposed change is fine, and would be excited to see them transitioned to actually-ESM later.

---

I didn't expect that the import style would be the contentious thing

[jakebailey]

would be excited to see them transitioned to actually-ESM later.

That's next up; DT is already mjs (woo)

[amcasey]

Don't block on me - I didn't read it carefully, but I have no objections.

[jakebailey]

All good, I was busy with something else and just hadn't yet hit the button.