Are the refactor and action properties needed? Or can we assume they are Move to file since we're making this call

Yes, I believe the refactor and the action properties are needed to compute the associated code action. getEditsForAction in types.ts matches the action property to the correct refactor action.

For this new API though, won't the action always be Move to file?

I said today that my API questions didn’t affect the protocol, but I was kind of wrong; there is a parallel. My suggestion on the LS side was to remove the move-to-file-specific LS method and allow an extra argument to be passed to the existing getEditsForRefactor method. The exact same possibility exists here: instead of making a new command, we could reuse the existing GetEditsForRefactor command if we modify its request type to allow an argument containing the targetFile property. @mjbvz thoughts?

I'm good with that. For the protocol, I'll just need the expected argument's type

Bugs and clarity nits:

1. Js isn’t the only JS extension—use isSourceFileJS(newSourceFile) or hasJsFileExtension(newFile) to cover cjs, mjs, jsx, etc.
2. Use newSourceFile.impliedNodeFormat instead of getImpliedNodeFormatForFile (the latter gets called once during program construction and gets cached on the former)
3. Since this is the case where the new file already exists, it seems like newSourceFile.commonJsModuleIndicator and externalModuleIndicator are relevant here.
4. Whatever the logic is, it has to be commented since it’s extremely hard to think about.
5. What about the case where the new file doesn’t exist? In that case, we can’t look at the new file’s existing syntax, but the other considerations still apply. There may need to be a test or two for that.

// Cannot use require if not JS
if (isSourceFileJs(newSourceFile) && (
    // Use require if detected CJS format
    newSourceFile.impliedNodeFormat === ModuleKind.CommonJS ||
    !newSourceFile.impliedNodeFormat && (
        // Target file is already using require,
        // or not using import and old file is using require
        newSourceFile.commonJsModuleIndicator ||
        !newSourceFile.externalModuleIndicator && oldSourceFile.commonJsModuleIndicator) ||
    // Must use require in CJS emit with verbatimModuleSyntax
    getEmitModuleKind(compilerOptions) === ModuleKind.CommonJS && compilerOptions.verbatimModuleSyntax
)) {
  useEsModuleSyntax = false;
}    

Style nit:

If sourceFile is undefined, something has gone horribly wrong since you were told that newFileExists and you shouldn’t continue. Wrap program.getSourceFile(newFile) in a Debug.checkDefined or something. And since you access it in an earlier if block, just do that at the very top of the function. (getSourceFile is pretty cheap but not free, and it’s nice to avoid repeating yourself.)

const targetSourceFile = newFileExists ? Debug.checkDefined(program.getSourceFile(newFile)) : undefined;

Then, you never have to check newFileExists again; you can just check if (targetSourceFile) and know it means the same thing, while narrowing undefined out of its type.

Clarity nit:

Comments in this function would be helpful:

1. Why is importAdder sometimes undefined?
2. Why might it throw when using it, and what are we doing to recover?

I’m pretty sure pathToNewFileWithExtension === fileName 100% of the time. Are you sure this is necessary? getModuleSpecifier just wants the file name of the file you’re importing, which you already have with fileName.

Style nit:

Same as before, if sourceFile is undefined, something has gone horribly wrong and you can’t do anything useful to recover, so there’s no need to code defensively here. (Also, hoist this program.getSourceFile out so you’re not retrieving it repeatedly in this nested loop.)

I've moved program.getSourceFile out but the sourceFile will be undefined if we are moving to a new file. So for recomputing the module specifier, if

• We move to an existing file, import adder will handle it
• If moving to a blank existing file, we will recompute it using getModuleSpecifier()
• If moving to a new file, it will get copied over as is because to use getModuleSpecifier() we need the targetSourceFile which is not available

Clarity nits:

1. Throughout this file, can we do a review of everywhere the word “new” is used and make sure it’s only getting used when the file is actually new? I propose to settle on the term “target file” to mean the file to which we’re moving code, whether it is new or existing. Everything here seems to be using the terminology “new” as a legacy from when this code was only used for a refactor called “Move to new file.”
2. This function seems to have a duplicated parameter, newFile and newFilename.
3. You could simplify the signature even more by removing newFileExists and making the type of targetFile be string | SourceFile. If it’s a string, it’s the file name for a new file. If it’s a SourceFile, it’s the existing target file.

program is already available to you without declaring this constant; see the other functions in this file as an example. This is similar to other places where I mentioned that code was unnecessarily defensive—if you find yourself with a type that says it could be undefined, but your function can’t do anything useful if that object is undefined, it’s worth figuring out whether it can actually be undefined in reality. Often, the answer is that if you have an undefined, some core invariant has already been violated, and it’s better to throw than silently do nothing. Presenting the user with an empty list of files isn’t going to help the user—they’re better off seeing VS Code present them an error message saying that something went wrong. And when that user files a bug, you’ll be very happy that they were able to paste a stack trace to exactly where something went wrong instead of just saying “I tried to move this code to another file and nothing happened 🤷”

This code iterates over the source files 3 times and creates and mutates a lot of intermediate arrays. It looks the filter, map, and for loop can be replaced with a single mapDefined call.

Don’t forget to simplify this after program is no longer typed as possibly undefined.

After fixing services.ts, can these return type properties ever actually be undefined?

Instead of deleting this test, you can modify it to use the TS Server command you expect VS Code to use now. (In doing so, you’ll find that you need another update to protocol.ts to allow the interactive refactor arguments.)