Skip to content

refactor!(NODE-3368): make name prop on error classes read-only #2879

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

Merged
merged 13 commits into from
Jul 7, 2021

Conversation

andymina
Copy link
Contributor

@andymina andymina commented Jul 2, 2021

Description

  • Added getter function to error classes to ensure that it is a read-only property
  • Added unit tests for .name property to ensure it is a read-only and matches the class name

@andymina andymina marked this pull request as ready for review July 2, 2021 21:12
@andymina andymina self-assigned this Jul 2, 2021
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Nice work! 🚀 just some suggestions we can discuss

@andymina andymina requested a review from nbbeeken July 6, 2021 14:17
@andymina andymina requested a review from nbbeeken July 6, 2021 14:25
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

LGTM

@nbbeeken nbbeeken added the Team Review Needs review from team label Jul 6, 2021
@nbbeeken nbbeeken requested review from dariakp and emadum July 6, 2021 16:34
this.address = pool.address;
}

get name(): string {
return 'PoolClosedError';
Copy link
Contributor

Choose a reason for hiding this comment

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

are the two renamings in this file intentional? these could potentially be breaking changes

Copy link
Contributor Author

@andymina andymina Jul 6, 2021

Choose a reason for hiding this comment

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

This was unintentional, but this did highlight another issue. The test should assert that the name prop matches the actual name of the class, but this isn't true for PoolClosedError and WaitQueueTimeoutError as both of their name props add the "Mongo" prefix. For now, I've imported both of those errors and aliased them with their "Mongo" prefix, but it's only a temporary fix to a potentially larger problem.

@andymina andymina requested a review from dariakp July 6, 2021 19:59
@nbbeeken nbbeeken changed the title refactor(NODE-3368): make name prop on error classes read-only refactor!(NODE-3368): make name prop on error classes read-only Jul 6, 2021
@andymina andymina requested a review from dariakp July 6, 2021 21:01
@nbbeeken nbbeeken merged commit 12d0060 into 4.0 Jul 7, 2021
@nbbeeken nbbeeken deleted the NODE-3368/4.0/error-name-readonly branch July 7, 2021 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants