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 errors array to hub api metadata and status to polling event metadata #425

Merged
merged 2 commits into from
Nov 16, 2020

Conversation

rgwozdz
Copy link
Contributor

@rgwozdz rgwozdz commented Nov 13, 2020

First off, apologies. This PR includes a lot of changes due to auto-lint fixing.

The actual purpose of the PR is to standardize the error events that get emitted if there was an error in the download export process. All error events look like:

{
 detail: { error, metadata }
}

To facilitate that, I have also changed the response of the hubDownloadMetadata method to include the errors array from the Hub API.

All other changes are tests and linting.

…ta. Ensure a status is added to

affects: @esri/hub-downloads
Copy link
Contributor

@r00b r00b left a comment

Choose a reason for hiding this comment

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

LGTM

/* ready, not_ready, creating, updating, failed*/
status: string,
/* ready, not_ready, creating, updating, failed */
status: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

why isn't this an enum?

Copy link
Member

Choose a reason for hiding this comment

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

or a type like 'ready' | 'not_ready' | 'creating' | 'updating' | 'failed'?

That's what we use in other places for this sort of thing.

@mikeringrose I still don't have good instincts on when to use one of the many ways (type, enum, const enum, etc) TS offers for this kind of situation. Worse, I'm sort of mentally blocked on it after having been burned by making the wrong choice over in rest-js. If you have experience/insight, I think it would be great to document some guidelines here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikeringrose - I'm using a DownloadStatus type now.

Copy link
Member

@tomwayson tomwayson left a comment

Choose a reason for hiding this comment

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

I'm worried by the linting changes. I fixed the broken pre-commit hook back in August, and cleaned up the exisitng linting errors: #314 That implies that these errors have slipped in since then?

@@ -98,11 +97,11 @@ class PortalPoller implements IPoller {
.catch((error: any) => {
if (error instanceof ExportCompletionError) {
eventEmitter.emit(`${downloadId}ExportError`, {
detail: { metadata: { errors: [error] } }
Copy link
Member

Choose a reason for hiding this comment

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

this makes it look like a breaking change. in order to avoid that we should probably keep metadata.errors (in addition to detail.error) until the next major release. You can add it to this list: #305

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the change to keep metadata.errors. As of this writing, the code snip above isn't updating to show it, but you can view it here.

…rs to avoid breaking change.

affects: @esri/hub-downloads
@rgwozdz
Copy link
Contributor Author

rgwozdz commented Nov 16, 2020

@tomwayson - I think the linting-issues slipped in quite a while ago. I believe they occurred on a commit in which I did not use npm run c, but rather did git commit.

Copy link
Member

@tomwayson tomwayson left a comment

Choose a reason for hiding this comment

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

LGTM

@rgwozdz rgwozdz merged commit 7d5f9b6 into master Nov 16, 2020
@rgwozdz rgwozdz deleted the p/download-errors branch November 16, 2020 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants