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 a TypeError instead of the generic Error #32585

Merged
merged 9 commits into from Jan 13, 2021
Merged

Conversation

rohit2sharma95
Copy link
Collaborator

@mdo
Copy link
Member

mdo commented Jan 5, 2021

/cc @XhmikosR for review

@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta2 via automation Jan 7, 2021
v5.0.0-beta2 automation moved this from Inbox to Approved Jan 7, 2021
XhmikosR
XhmikosR previously approved these changes Jan 7, 2021
@XhmikosR
Copy link
Member

XhmikosR commented Jan 7, 2021

@rohit2sharma95 actually, shouldn't this test fail right now?

expect(() => {
Util.typeCheckConfig(namePlugin, config, defaultType)
}).toThrow(new Error('COLLAPSE: Option "parent" provided type "number" but expected type "(string|element)".'))

Perhaps we should use toThrowError?

@XhmikosR XhmikosR dismissed their stale review January 7, 2021 07:49

See last comment

v5.0.0-beta2 automation moved this from Approved to Review Jan 7, 2021
@@ -263,7 +263,7 @@ class Dropdown extends BaseComponent {
typeof config.reference.getBoundingClientRect !== 'function'
) {
// Popper virtual elements require a getBoundingClientRect method
throw new Error(`${NAME}: Option "reference" provided type "object" without a required "getBoundingClientRect" method.`)
throw new TypeError(`${NAME}: Option "reference" provided type "object" without a required "getBoundingClientRect" method.`)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should make the related tests stricter, probably makes sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.
Also, changed the NAME to upper case to make the consistency.
Should check for the same in jQueryInterface also? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean should the tests be updated here? (To check if the error is the instance of TypeError)

try {
jQueryMock.fn.dropdown.call(jQueryMock, action)
} catch (error) {
expect(error.message).toEqual(`No method named "${action}"`)
}

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah let's switch any other instances to toThrowError

rohit2sharma95 and others added 3 commits January 8, 2021 00:26
Convert the `NAME` to upper case to make the consistency in the error
message
@XhmikosR XhmikosR marked this pull request as draft January 13, 2021 11:15
@rohit2sharma95 rohit2sharma95 marked this pull request as ready for review January 13, 2021 17:01
@XhmikosR XhmikosR changed the title Change from Error to TypeError Throw a TypeError instead of the generic Error Jan 13, 2021
v5.0.0-beta2 automation moved this from Review to Approved Jan 13, 2021
@XhmikosR XhmikosR merged commit c9cd741 into main Jan 13, 2021
v5.0.0-beta2 automation moved this from Approved to Done Jan 13, 2021
@XhmikosR XhmikosR deleted the rohit/main/type-error branch January 13, 2021 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5.0.0-beta2
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants