Fix crash when there are multiple packages to install per code fix action #56400
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
| "response": { | ||
| "changes": [], | ||
| "commands": [ | ||
| { |
There was a problem hiding this comment.
Should these be deduplicated? If yes, at the end or during addition?
(irrespective of that we do need the fix to TI adapter to handle the correct promise resolution)
There was a problem hiding this comment.
Probably? I'm not familiar enough with this to know for sure...
There was a problem hiding this comment.
I'm guessing that this will run two npm installs, at once probably, and should be deduped. It seems safer, and just as easy, to do it on addition, but I could be wrong. It looks like the key of the new Map might need to be based on the package name instead of a numeric id.
@zkat worked on this code much more recently than I have and might have additional thoughts.
There was a problem hiding this comment.
its possible to have duplicate "npm install requests" if i clicked "fix all" command multiple times right? So it has to be numeric id and installer handles the multiple installs for same package
There was a problem hiding this comment.
The deduped result would be nice but wondering if it really matters
There was a problem hiding this comment.
It'd be equivalent to running two npm install X commands at once, right? If a manual test of that shows that npm handles that case on its own, I think it's fine to have ATA allow dupes.
Fixes #56266