Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing auto import suggestions for UMD modules such as classnames #41761

Closed
OliverJAsh opened this issue Dec 1, 2020 · 11 comments · Fixed by #42141
Closed

Missing auto import suggestions for UMD modules such as classnames #41761

OliverJAsh opened this issue Dec 1, 2020 · 11 comments · Fixed by #42141
Labels
Bug A bug in TypeScript Domain: Auto-import Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this
Milestone

Comments

@OliverJAsh
Copy link
Contributor

  • VSCode Version: 1.51.1
  • OS Version: 11.0.1

Steps to Reproduce:

import * as React from 'react';

const el = <div className={REPLACE_ME}>foo</div>;
  1. yarn add @types/classnames
  2. Erase REPLACE_ME and begin typing classNames

Does this issue occur when all extensions are disabled?: Yes

Expected: VS Code suggests an auto import of classNames from module 'classnames'.

Actual: no suggested auto import.

image

I believe this is something to do with the fact that classnames is defined as a UMD module. If I change the type definitions for classnames to remove the UMD definition, the auto import does work correctly:

image


Note that VS Code does correctly provide import suggestions after we've finished typing the identifier, when we move our cursor.

image

This issue is specifically about auto imports inside the suggestions that appear as you're typing.

@mjbvz mjbvz transferred this issue from microsoft/vscode Dec 1, 2020
@mjbvz mjbvz removed their assignment Dec 1, 2020
@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Dec 3, 2020
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Dec 3, 2020
@DanielRosenwasser DanielRosenwasser added Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this labels Dec 4, 2020
@aminpaks
Copy link
Contributor

aminpaks commented Dec 17, 2020

Would someone guide me how I can easily get to a point to reproduce this locally?

Not very familiar how code fixes work for node_modules/@types/... and requesting a code completion at a specific point.

Found how to work with completions in cases/fourslash/tsxCompletionXX.ts

@aminpaks
Copy link
Contributor

aminpaks commented Dec 27, 2020

It turns out this case happens cause the completion ends up within a {..} and current implementation looks up for destructuring of an object.

Trying to find a good spot to add the completion logic...

Update: the comment above only happens if the file extension is .js including JSX... Maybe we should look into that as well but I wanna focus on a fix of auto import for the global module.

@aminpaks
Copy link
Contributor

aminpaks commented Dec 27, 2020

@OliverJAsh is the issue occurs within JSX/TSX language variant or JS?

Ya I can finally reproduce this constantly. As you mentioned it's related to CommonJS module as they have the following export declaration

declare const someModule: () => void;
export = someModule;

@OliverJAsh
Copy link
Contributor Author

@OliverJAsh is the issue occurs within JSX/TSX language variant or JS?

I think I only tested it with TSX. Not sure if it occurs in other languages.

@aminpaks
Copy link
Contributor

@OliverJAsh would you also provide a simplified version of your TS config? specially how you configured the modules.

there are settings that I'm interested to see:
https://www.typescriptlang.org/tsconfig#module
https://www.typescriptlang.org/tsconfig#esModuleInterop
https://www.typescriptlang.org/tsconfig#allowSyntheticDefaultImports

@aminpaks
Copy link
Contributor

aminpaks commented Dec 28, 2020

To be honest at this point I'm not sure if this is a bug or the correct behaviour. The code fix works though I'm not sure if auto import should be provided or even how auto import works in TypeScript/VSCode.

I would need some help to investigate more.

Added a new fourslash test here:
#42141

@OliverJAsh
Copy link
Contributor Author

OliverJAsh commented Dec 29, 2020

@aminpaks I am using the default settings as I have no tsconfig.json. Repo with reduced test case: https://github.com/OliverJAsh/ts-umd-auto-import-suggestions-bug.

@aminpaks
Copy link
Contributor

@OliverJAsh yes this is exactly what I observed by looking at the code and writing that unit-test.
If you try to write class VSCode will offer you classNames as a completion, after you write full classNames TypeScript will offer you a code fix to add classnames as a new import.

Here I'm not sure if auto import should (if yes what is the default behaviour) be provided when you complete writing classNames and press enter.

image

@OliverJAsh
Copy link
Contributor Author

Here I'm not sure if auto import should (if yes what is the default behaviour) be provided when you complete writing classNames and press enter.

Most users nowadays choose to import UMD modules rather than relying on the globals provided by the UMD (that's the best practice now) so I think the majority of users would find this useful.

Just to make sure we're on the same page, this is what I'm asking for: the import should be suggested in the list as the user types, in the same way that all other non-UMD modules appear in the list.

image

@aminpaks
Copy link
Contributor

aminpaks commented Dec 29, 2020

@OliverJAsh that's exactly what I expect to happen. As you can see in the test that I wrote it checks for both. Auto import by completion and code fixes.

I don't know why in provided sample code auto import stops working though!

@aminpaks
Copy link
Contributor

@RyanCavanaugh I would need some help to continue working on this issue. If someone on the team would clarify how TypeScript determines auto imports from node_modules/@types in code completion would really help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Auto-import Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this
Projects
None yet
5 participants