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

Add exported members of all project files in the global completion list #13921

Closed
wants to merge 9 commits into from

Conversation

zhengbli
Copy link
Contributor

@zhengbli zhengbli commented Feb 7, 2017

Fixes #7849

This PR adds several things:

  1. It adds the exported members of all project files in the global completion list, no matter if the file was imported by the current file or not;
  2. however, for members coming from not-yet-imported file, the returned CompletionEntry now has a new property hasAction, indicating if the user commits it, additional code actions will be needed to avoid errors;
  3. after emitting a completion entry with hasAction set to true, the editor can send a CommitCompletionWithCodeAction request to the server to get the corresponding code actions, and then apply them just like other code actions.

@zhengbli
Copy link
Contributor Author

zhengbli commented Feb 8, 2017

@mhegazy @vladima comments?

@thekalinga
Copy link

@zhengbli Does this include files under node_modules aswell?

@zhengbli
Copy link
Contributor Author

Yes as long as they have been used in the project at least once

@thekalinga
Copy link

Will the first time import of a class work (this class has not been imported into my source, but the node_modules contains the definition file for this class I'm trying to import)?

@zhengbli
Copy link
Contributor Author

zhengbli commented Feb 14, 2017

No you have to import that module at least once anywhere in your project, so that module becomes part of your project. Otherwise it would be too expensive to search all the folders all the time. The same applies to how we implements the current quick fix now.

@thekalinga
Copy link

thekalinga commented Feb 14, 2017

@zhengbli The sheer mention of a dependency in package.json & the presence of the dependency in node_modules implies that the user is wanting to use the module. Most of the time (~99%) user does not modify any file under node_modules manually.

Only npm (or) yarn would trigger a change to the node_modules directory structure, which user would not run very frequently in the project.

So the amount of change cycles that trigger autocomplete index to change are relatively limited from the point of view of the files under node_modules.

Unless I'm missing something, forcing user to import at least once in the project is an arbitrary expectation.

Please let me know if I've missed something

@zhengbli
Copy link
Contributor Author

I agree parsing package.json is a good estimate. Though that belongs to a bigger question: whether we should eagerly include modules listed in package.json into the program, which would have impacts on not only completion list, also other features like navigate-to. The changes made in this PR serves as the first step towards and is building based on how the program is currently constructed. That proposed changes to make the program bigger eagerly may go to a separate PR later.

@thekalinga
Copy link

thekalinga commented Mar 17, 2017

@zhengbli Is this change going to be part of 2.3?

@zhengbli
Copy link
Contributor Author

@thekalinga at this point I'm not sure, depends on if the team has the time to review the changes

@zhengbli zhengbli changed the title Add exported members of all project files in the global completion list [WIP] Add exported members of all project files in the global completion list Apr 3, 2017
@zhengbli zhengbli changed the title [WIP] Add exported members of all project files in the global completion list Add exported members of all project files in the global completion list Apr 5, 2017
@zhengbli zhengbli requested review from mhegazy and billti April 5, 2017 10:06
@zhengbli
Copy link
Contributor Author

zhengbli commented Apr 5, 2017

Updated with the feature embedded in the "CompletionEntryDetails" API call.

@mjbvz
Copy link
Contributor

mjbvz commented Apr 13, 2017

The API looks good to me for VSCode. Looking forward to picking up this change

@angelozerr
Copy link

I'm looking for too for this great feature to integrate it inside Eclipse. Do you know when you think you could merge this issue to master to consume it with typescript@dev ?

@angelozerr
Copy link

@mhegazy do you know when this PR will be accepted? The auto import feature is not in the RoadMap Do you know for which version of TypeScript, it will be available? Thanks!

@minestarks
Copy link
Member

This is redone in #17851

@minestarks minestarks closed this Aug 28, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Oct 17, 2017

Merged in #19069. thanks @zhengbli !

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants