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

Throw specific error keys if available for getTimetableForWeek #85

Merged
merged 2 commits into from
Oct 17, 2022
Merged

Throw specific error keys if available for getTimetableForWeek #85

merged 2 commits into from
Oct 17, 2022

Conversation

wolflu05
Copy link
Contributor

@wolflu05 wolflu05 commented Oct 7, 2022

I need to differentiate if the error is a not allowed error or an other error, so I added this check and throw the key directly.

index.js Outdated
@@ -586,6 +586,13 @@ class WebUntis {
});

if (typeof response.data.data !== 'object') throw new Error('Server returned invalid data.');

if (response.data.data.error && response.data.data.error.data && response.data.data.error.data.messageKey)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to use optional chaining.

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 thought we should keep backwards compatibility for node < 14 because optional chaining isn't used for the rest of the code. But if you want I'll add that and add a min version in the package.json.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is true. But I am not sure why we should keep the backwards compatibility with node 14. Node 14 is in maintenance mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm totally fine with either. Just tell me what you want I should do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then go ahead with optional chaining.

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 also refactored to use a specific property code on the error instead of using the error message for the error code and also refactored the optional chaining below in this function.

@TheNoim TheNoim merged commit 849ae59 into SchoolUtils:master Oct 17, 2022
@wolflu05
Copy link
Contributor Author

Thank you for merging this.

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