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

fix: add error state to onAfterDownload #42

Merged
merged 13 commits into from
Jun 22, 2022

Conversation

jackiewmacharia
Copy link
Contributor

🎉 Thanks for sending this pull request! 🎉

Please make sure the title is clear and descriptive.

If you are fixing a typo or documentation, please skip these instructions.

Otherwise please fill in the sections below.

Which problem is this pull request solving?

Adds error state to onAfterDownload to allow for icon change in case there is an issue with deno binary download or installation

List other issues or pull requests related to this problem

Checklist

Please add a x inside each checkbox:

  • I have read the contribution guidelines.
  • The status checks are successful (continuous integration). Those can be seen below.

A picture of a cute animal (not mandatory but encouraged)

@jackiewmacharia jackiewmacharia requested a review from a team June 9, 2022 14:50
@github-actions github-actions bot added the type: bug code to address defects in shipped code label Jun 9, 2022
Copy link
Contributor

@danez danez left a comment

Choose a reason for hiding this comment

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

LGTM

I was wondering if it would make more sense to supply the error directly to the function instead of the boolean, but there is only one kind of error and I also don't really see a usecase for that. So I think this is fine the way it is.

src/bridge.ts Outdated Show resolved Hide resolved
@eduardoboucas
Copy link
Member

LGTM

I was wondering if it would make more sense to supply the error directly to the function instead of the boolean, but there is only one kind of error and I also don't really see a usecase for that. So I think this is fine the way it is.

Agreed! It seems to me that the amount of work of adding a boolean and an error is similar, and the latter gives us more flexibility going forward, so I'd recommend doing it now.

danez
danez previously approved these changes Jun 14, 2022
@danez danez self-requested a review June 14, 2022 13:58
src/bridge.ts Outdated Show resolved Hide resolved
src/bridge.ts Outdated Show resolved Hide resolved
src/bridge.ts Outdated Show resolved Hide resolved
src/bridge.ts Outdated Show resolved Hide resolved
src/bridge.ts Outdated Show resolved Hide resolved
src/bridge.ts Outdated Show resolved Hide resolved
@jackiewmacharia jackiewmacharia merged commit 2cb24ac into main Jun 22, 2022
Skn0tt pushed a commit to netlify/build that referenced this pull request Apr 23, 2024
* fix: add error state to after download

Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants