Filter out global keywords of UMD module export declarations in completion providing auto import suggestions

[aminpaks]

Please verify that:

• There is an associated issue in the Backlog milestone (required)
• Code is up-to-date with the master branch
• You've successfully run gulp runtests locally
• There are new or updated unit tests validating the change

Fixes #41761

[sandersn]

From reading the issue thread, this test incorrectly passes, right? As I understood it, you wanted to create a failing test to match your observations in the editor. Is that correct?

[aminpaks]

@sandersn

Yes. I don't quite understand why it works in the test but in a real env it fails (or at least in some cases fail.)

I mentioned my observation here

[andrewbranch]

What Oliver said is that it fails when it’s declared as a UMD module, which this isn’t. You’d need to add export as namespace classNames too.

[andrewbranch]

@aminpaks it sounds like you’re still working on this—do you want to convert the PR to a draft so it’s clear it’s not done?

[aminpaks]

@andrewbranch Done. I'll look into that asap.

[aminpaks]

@andrewbranch would you tell me what is the right test case for this issue? I can't seem to reproduce it.

Update: Uh I think I understand why this test passes cos I removed the auto import suggestion from the test.

1. Should this test only verify the auto import suggestions and not import fixes?
2. Should it support partial word auto import suggestions or only at classNames should suggest the import?

// @filename: /SomeReactComponent.tsx
//// import * as React from 'react';
////
//// const el1 = <div className={class/*1*/}>foo</div>;

goTo.marker("1");

verify.completions({
  includes: [{
    name: "classNames",
    hasAction: true,
    source: "/node_modules/@types/classnames/index",
    sortText: completion.SortText.AutoImportSuggestions,
  }],
  preferences: {
      includeCompletionsForModuleExports: true,
  }
});

[andrewbranch]

1. My understanding from Missing auto import suggestions for UMD modules such as classnames #41761 was that the import fix works but the completions suggestion doesn’t. That’s the bug.
2. Auto-import completions don’t need the whole identifier to be typed.

would you tell me what is the right test case for this issue

Figuring this out tends to be 90% of the work for these kinds of bugs 😄

[aminpaks]

@andrewbranch

I think I found the issue. When we add export as namespace classNames; to the module the modules carries some extra property called globalExports and then at this line it counts as a global keyword and then at this line it shadows the one that actually carries the origin with it and we lose track of the origin and that causes this line to have no action to provide the import.

I just don't have enough insights to provide a fix in the right spot that said I think we could exclude it from the global or keywords if it already has an export, what do you think?

[andrewbranch]

I think for UMD exports, the global should be shadowed / filtered out if the file is already a module, unless --allowUmdGlobalAccess is set. If the file is a script or --allowUmdGlobalAccess is set, I guess there isn’t a great solution, so I would keep it as is (the global wins).

[aminpaks]

Gotcha, so would you tell me if it's okay to remove the module from global symbols if it has exports?
I found this (refer to "^^" down below) in the checker.ts that is adding the module to global symbols (apologies github doesn't allow links to that file).
Of course we must add the logic to check for allowUmdGlobalAccess compiler option also that I don't know if that's possible at this spot

            for (const file of host.getSourceFiles()) {
                if (file.redirectInfo) {
                    continue;
                }
                if (!isExternalOrCommonJsModule(file)) {
                    // It is an error for a non-external-module (i.e. script) to declare its own `globalThis`.
                    // We can't use `builtinGlobals` for this due to synthetic expando-namespace generation in JS files.
                    const fileGlobalThisSymbol = file.locals!.get("globalThis" as __String);
                    if (fileGlobalThisSymbol) {
                        for (const declaration of fileGlobalThisSymbol.declarations) {
                            diagnostics.add(createDiagnosticForNode(declaration, Diagnostics.Declaration_name_conflicts_with_built_in_global_identifier_0, "globalThis"));
                        }
                    }
                    mergeSymbolTable(globals, file.locals!);
                }
                if (file.jsGlobalAugmentations) {
                    mergeSymbolTable(globals, file.jsGlobalAugmentations);
                }
                if (file.patternAmbientModules && file.patternAmbientModules.length) {
                    patternAmbientModules = concatenate(patternAmbientModules, file.patternAmbientModules);
                }
                if (file.moduleAugmentations.length) {
                    (augmentations || (augmentations = [])).push(file.moduleAugmentations);
                }
                if (file.symbol && file.symbol.globalExports) {
                // ^^ this line adds the module as part of global symbols
                // adding !isExternalOrCommonJsModule(file) + the check for allowUmdGlobalAccess would fix it
                    // Merge in UMD exports with first-in-wins semantics (see #9771)
                    const source = file.symbol.globalExports;
                    source.forEach((sourceSymbol, id) => {
                        if (!globals.has(id)) {
                            globals.set(id, sourceSymbol);
                        }
                    });
                }
            }

Update: We can't add the logic there in the checker. I didn't quite get where you meant to put the logic at first but now that I think about it we should alter the shadowing logic and make it work the way you describe it

[andrewbranch]

Yeah, I’m only talking about changes to completions.ts.

[aminpaks]

How do you feel about this fix @andrewbranch?

https://github.com/microsoft/TypeScript/pull/42141/files#diff-4237fd08a5f920543e184b7748bc8ae000d9b1c031810ba375b0053362ec2b6aR1530

Added two test cases, one to assert auto suggestion for module exports and the other for global keywords.

[andrewbranch]

Looks good after these small nits

[andrewbranch]

Thanks @aminpaks!

[aminpaks]

My pleasure @andrewbranch!