-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Bundle fileName with CodeActionCommand #19881
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
Conversation
release 2.6 as well. |
@mhegazy Should we change the services.ts API as well? |
We have not released since we bumped it to 0.7 |
//cc @mjbvz |
src/services/types.ts
Outdated
export interface InstallPackageAction { | ||
/* @internal */ | ||
export interface CodeActionCommandBase { | ||
file: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might the project name be required as well if the same file appears in multiple projects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it will be required if we are going to call updateTypingsForProject
(which calls delayUpdateProjectGraphAndInferredProjectsRefresh
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an opaque object so we can change this if we turn out to need that.
I think we should update all projects that include that file, since they all might be affected by a new directory in node_modules
. They should already be watching that directory though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have thought PackageInstalledResponse
would behave more or less the same as SetTypings
, which is a ProjectResponse
.
bd1594f
to
4e59232
Compare
@Andy-MS please port this to release-2.6 |
@Andy-MS Is this ready to merge? I'd like to code against it on the VS side. |
* Bundle fileName with CodeActionCommand * Update test * Fix API tests * Add new overloads in services * Fix overload * Update API baselines
* origin/master: (140 commits) test overriding Session.event Update editorServices.ts Fix semantic merge conflict (microsoft#20119) LEGO: check in for master to temporary branch. Moved minified file exclusion Fixed internal safelist For import completion, if multiple re-exports exist, choose the one with the shortest path (microsoft#20049) Bundle fileName with CodeActionCommand (microsoft#19881) Simplify documentHighlights (microsoft#20091) LEGO: check in for master to temporary branch. Support semantic classification of alias (microsoft#20012) In `getContextualTypeForBinaryOperand`, only need to look for `=` assignment operator, not e.g. `+=` (microsoft#20037) lineAction: Use an enum instead of true | false | undefined (microsoft#20086) LEGO: check in for master to temporary branch. cleanup NodeTypingsInstaller remove comments type `event` callback correctly update baselines defer callback and remove handler object Support arbitrary prototype property assignments in navigation bar (microsoft#19923) ...
Fixes #19845
There is also a method in
services.ts
that still takes afileName: string
as the first argument; it would be backwards-incompatible to change this so not sure if we should do that? We could leave it as a deprecated overload though.