Fix foreach bug #925
Fix foreach bug #925
Conversation
dilin-MS
force-pushed
the
bug-fix-foreach
branch
from
74ed4fa to
222ba49
Compare
dilin-MS
force-pushed
the
bug-fix-foreach
branch
from
023375d to
04dff2a
Compare
| @@ -66,8 +66,8 @@ export abstract class ContainerDeviceBase implements Device { | |||
| return this.componentType; | |||
| } | |||
|
|
|||
| async checkPrerequisites(): Promise<boolean> { | |||
| return true; | |||
| async checkPrerequisites(): Promise<void> { | |||
There was a problem hiding this comment.
Why not using the same signature as the other component? Then we can manage the checkPrerequisites as an interface.
E.g. in ArduinoDeviceBase.ts, the signature is async checkPrerequisites(operation: string): Promise<void>
There was a problem hiding this comment.
I agree to have an interface like ExtensionDependent (you could find a proper name), which has methods "isExtensionAvailable" instead of "checkPrerequisities" and throw DependentExtensionNotFoundError.
In this way, any component which need installing dependent extension should implement this interface, such as
abstract class ContainerDeviceBase implement Device, ExtensionDependent {
There was a problem hiding this comment.
Currently checkPrerequisites(operation: string): Promise<void>; is the required method of interface Component.
Both ContainerDeviceBase and ArduinoDeviceBase needs to implement this method.
Is it necessary to move checkPrerequisite() to be another interface like ExtensionDependent?
For the different method signature:
- ts allows ignoring extra function parameters. More info see link.
parameter operation: stringdoes appears because they are not needed in the implementation of the function. eslintdoes not allow unused vars.
| @@ -25,12 +25,10 @@ export class RemoteExtension { | |||
| return await checkExtensionAvailable(ExtensionName.Remote); | |||
| } | |||
|
|
|||
| static async checkRemoteExtension(): Promise<void> { | |||
| static async checkRemoteExtension(operation: string): Promise<void> { | |||
There was a problem hiding this comment.
nit: can we manage the "operation" as enum type here?
It can provide us an overview of what kind of operations will need to check remote extension. Another benefit to use enum type here is improve the re-usability for the same operation used in multiple places.
There was a problem hiding this comment.
The Operation: string is used when in error message to help know what operation throws error. The value of operation varies a lot through the whole project. This is why I didn't set it as enum at the very beginning.
If we have a enum type for all operations, the list will be very large.
E.g. "create function application", "get credential from subscription id", "crete iot containerized project", "open project in remote", "configure project environment", "generate CRC", etc.
Really a big list since operations varies.
I agree that using a enum type to represent operation helps to maintain the code and improve the re-usability of operation string. Let's discuss further for a better implemention and we can set up a new PR to fix that avoid messing up this PR with too many changed files.
| @@ -84,8 +85,11 @@ export abstract class ArduinoDeviceBase implements Device { | |||
| return await checkExtensionAvailable(ExtensionName.Arduino); | |||
| } | |||
|
|
|||
| async checkPrerequisites(): Promise<boolean> { | |||
| return await ArduinoDeviceBase.isAvailable(); | |||
| async checkPrerequisites(operation: string): Promise<void> { | |||
There was a problem hiding this comment.
Same here as I comment in the checkRemoteExtension api.
| @@ -76,7 +75,7 @@ export class IoTContainerizedProject extends IoTWorkbenchProjectBase { | |||
| openInNewWindow: boolean | |||
| ): Promise<void> { | |||
| // Can only create project locally | |||
| await RemoteExtension.checkRemoteExtension(); | |||
| await RemoteExtension.checkRemoteExtension("create iot containerized project"); | |||
There was a problem hiding this comment.
The same operation string appears in many places.
dilin-MS
force-pushed
the
bug-fix-foreach
branch
from
04dff2a to
3d5f4a9
Compare
In TypeScript, usage of
foreachshould be careful. If function insideforeachis asynchronous,foreachwill not wait until all items are finished. (See Reference).this PR:
Promise.all(). Fix the bug.checkPrerequisite()function to returnPromise<void>instead ofPromise<boolean>. User will get error message when operation failed for not finding dependent extensions.