-
Notifications
You must be signed in to change notification settings - Fork 384
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Remove errant export of GetByRoleMatcher, fixing type checking in some TS configurations #575
fix: Remove errant export of GetByRoleMatcher, fixing type checking in some TS configurations #575
Conversation
Mixing export type with export = matchers can throw off TypeScript in some configurations. We do not need to export the matcher as a separate type, since the argument types are available in the matcher already.
types/matchers.d.ts
Outdated
@@ -1,9 +1,5 @@ | |||
import {ARIARole} from 'aria-query' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be changed to?
import {type ARIARole} from 'aria-query'
Follow-up: we found in #574 that Should we merge this to fix current breakage? Gentle ping @gnapse 馃槍 |
馃帀 This PR is included in version 6.4.2 馃帀 The release is available on: Your semantic-release bot 馃摝馃殌 |
What:
Follow-up to #143 , and the issue reported at #574.
Mixing
export type
withexport = matchers
can throw off TypeScript in some configurations. We do not need to export the matcher as a separate type, since the argument types are available in the matcher already.I will admit, I do not know the exact combination of configuration that causes this issue. I think it would be desirable to include a test case for it, to ensure that we do not break it accidentally in the future.
The types are doing quite a lot of heavy lifting (types for explicit extend, types on globals/declaration merging, plus being "external" to the source), so perhaps we are not testing all the combinations. I am a bit surprised that we did not get this error in the existing tests, the double export sounds conceptually invalid. The issue at #574 has a reproduction, but paring it down to the size of the existing tests might be best.
Anyway, in terms of compatibility, I am confident that this is the best approach. By avoiding the extra
export
, we do not confuse TypeScript, and we are back in the previous status quo (exports = matchers
only).Why:
Avoiding unintended type changes, in some environments.
How:
By reverting what the TypeScript error was pointing at:
Checklist:
I think this is ready to be merged, but the lack of an inverse test case is bothering me 馃槄
Please let me know if I can help in any way, and I'll give it a shot!