-
Notifications
You must be signed in to change notification settings - Fork 4.7k
A11y: Add types #19096
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
A11y: Add types #19096
Conversation
packages/a11y/src/addContainer.js
Outdated
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.
This may be enough of an edge case that we could use a JSDoc cast:
| document.querySelector( 'body' )?.appendChild( container ); | |
| /** @type { HTMLBodyElement } */ ( document.querySelector( 'body' ) ).appendChild( container ); |
The optional chain does seem to be correct, it's possible to have a document with no body:
document.createDocumentFragment().querySelector( 'body' ).appendChild( document.createElement( 'div' ) )
// Uncaught TypeError: Cannot read property 'appendChild' of null
That is an extreme edge case, however 🙂
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.
Whoops, I thought optional chaining was available already.
This change is blocked on #18939 or an alternative implementation.
packages/a11y/src/index.js
Outdated
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.
I removed the reassignment of the local variables which seems to serve no purpose here.
8f27e9d to
992c5e9
Compare
|
Rebased and feedback addressed. |
992c5e9 to
6efcba4
Compare
|
This has some an issue with tsc runs: We'll need to figure out how to make tsc aware of other packages, likely along the lines of #18838 (comment) |
6efcba4 to
c87f3e2
Compare
tsconfig.json
Outdated
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.
This fixes the issues with the unfound @wordpress/dom-ready module and doesn't seem to introduce any issues.
Seems sufficient for now.
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.
This fixes the issues with the unfound
@wordpress/dom-readymodule and doesn't seem to introduce any issues.Seems sufficient for now.
Related: #18927
Once this is in place, we'll probably have trouble adding types to anything which depends on another package which isn't type-ready (i.e. if there are any errors in the dependency code).
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.
we'll probably have trouble adding types to anything which depends on another package which isn't type-ready
Based on my experience here, it's already a problem because tsc can't locate the modules.
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.
Would you prefer to sort that out in #18927 or shall we include it here and merge this?
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.
12072f3 to
8003ce6
Compare
8003ce6 to
5eed3ea
Compare
|
I've rebased, this should be ready to land now. |
Description
Part of #18838
Extracted from #18942
Add types to
a11ypackage.Testing Instructions:
Verify type checking passes: