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

Adding optional error to KeyStatusesChangedEvent type #3541

Merged
merged 4 commits into from
Mar 2, 2021

Conversation

ed0wolf
Copy link
Contributor

@ed0wolf ed0wolf commented Feb 17, 2021

The ProtectionController can emit this event with an error, when the current license expires:

  1. eventBus.trigger(events.KEY_STATUSES_CHANGED, { data: null, error: e.error });

@dsilhavy dsilhavy added this to the 3.2.1 milestone Feb 17, 2021
@dsilhavy dsilhavy self-requested a review February 17, 2021 18:04
index.d.ts Outdated Show resolved Hide resolved
@dsilhavy dsilhavy modified the milestones: 3.2.1, 3.3.0 Feb 22, 2021
@dsilhavy
Copy link
Collaborator

@ed0wolf can you address my comment?

@dsilhavy
Copy link
Collaborator

@ed0wolf Thanks for the changes.

Following our contribution guidelines, and since this seems to be your first PR, could you please send me a signed copy of dash.js feedback agreement?

@ed0wolf
Copy link
Contributor Author

ed0wolf commented Feb 26, 2021

Apologies for the delay.

I was a bit confused why the other Events didn't have the correct error type, figured someone knew better.. 🤷

I looked through those events and fixed the the error type for the events which get emitted with DashJSError.

I also noticed that DashJSError doesn't extend Error this means err instanceof Error === false, which doesn't seem obvious to me.

Thanks!

@ed0wolf
Copy link
Contributor Author

ed0wolf commented Feb 26, 2021

@dsilhavy Where should I email the Feedback Agreement?

@dsilhavy
Copy link
Collaborator

dsilhavy commented Feb 26, 2021

Via email is fine: edit

@dsilhavy dsilhavy merged commit c7f6772 into Dash-Industry-Forum:development Mar 2, 2021
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.

None yet

2 participants