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

Finally, a progress indicator #69

Merged
merged 4 commits into from
Dec 11, 2019
Merged

Conversation

DarthHater
Copy link
Member

@DarthHater DarthHater commented Dec 10, 2019

A lot of the scans for bigger projects can take quite a while, and we weren't really giving the user any sort of visual feedback as to this.

This PR implements window.withProgress() such that things still remain async, but we are able to report progress appropriately.

This hurt my head more than I'd like to admit :)

OSS Index successful scan:
ossindexsuccess

IQ Server successful scan:
iqserverscan

@@ -69,7 +69,7 @@ export class NexusExplorerProvider
});
}

private async reloadComponentModel() {
private async reloadComponentModel(): Promise<any> {
await this.componentModel.evaluateComponents();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's async, do we need await here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

async allows you to await! (I'm not sure, I'll take a look tomorrow)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In checking, I decided to nix it, you can see that in my follow up commits.


for (let resultEntry of resultData.results) {
let componentEntry: ComponentEntry | undefined;
progress.report({message: "Reticulating splines...", increment: 25});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL glad you got got it

this._onDidChangeTreeData.fire();
}
});
this.reloadComponentModel()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got rid of v as we were never using it, and we return mostly empty promises to begin with (just like my parents!)

@@ -50,62 +50,84 @@ export class IqComponentModel implements ComponentModel {
return new Promise((c, e) => "my stubbed content entry");
}

public async evaluateComponents() {
public evaluateComponents(): Promise<any> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We technically didn't need to mark any of these as async, since we have a promise being returned higher up. I don't really like how many chains deep we have it right now (we can likely simplify quite a bit of this), but may as well remove these until we have an actual need to use await in the function. I mark the methods as Promise<any> so someone can at least see what's going on :)


let data: any;
window.withProgress(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was very fun to figure out, but after playing with it, I love it! What a weird experience! I had to use the .then() on this Thenable, because errors were getting swallowed by withProgress). This actually makes the code a bit more readable than it was before (at least in my opinion)

let password = configuration.get("ossindex.password") + "";
this.requestService = new OssIndexRequestService(username, password);
}
constructor(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed spacing while I was at it.

@@ -25,48 +25,69 @@ import { ComponentEntry } from "./ComponentEntry";

export class OssIndexComponentModel implements ComponentModel {
components = new Array<ComponentEntry>();
requestService: LiteRequestService;
dataSourceType: string = "ossindex";
requestService: LiteRequestService;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed spacing.

if (err) {
reject(`Unable to retrieve Application ID: ${err}`);
})
.on('response', (res) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched to using the .on('message', (handler) => {}) request for the request lib, as this is I think how it's preferred to use it. It certainly makes the flow read a bit easier, and leans on the framework. I mostly did this trying to figure out how to throw an error, etc.... but I liked the change enough to keep it.

});
}).on('error', (err) => {
console.debug(err);
if (err.message.includes('ECONNREFUSED')) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to do this ugly hack because the err Error type doesn't have code on it at compile time, but does at runtime. 🤷‍♂

@@ -16,7 +16,7 @@ import { BaseRequestService } from "./BaseRequestService";
* limitations under the License.
*/
export interface RequestService extends BaseRequestService {
getApplicationId(applicationPublicId: string): Promise<any>;
getApplicationId(applicationPublicId: string): Promise<string>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lil tiny thing to make this app better, try and switch away from using any all the time and actually use TypeScript :)

Copy link
Contributor

@allenhsieh allenhsieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- like how you commented on your own code and will steal that for personal use in future.

@DarthHater DarthHater merged commit d7a68d0 into master Dec 11, 2019
@DarthHater DarthHater deleted the WithProgressFunTimeExplosion branch December 11, 2019 18:55
bhamail pushed a commit that referenced this pull request Dec 11, 2019
# [0.6.0](v0.5.3...v0.6.0) (2019-12-11)

### Features

* Improve UX of Scans, etc... so that the user knows what is going on ([#69](#69)) ([d7a68d0](d7a68d0))
@bhamail
Copy link
Contributor

bhamail commented Dec 11, 2019

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants