@axe-core/react: fallback on _reactInternals #437
Conversation
|
|
| const message: string = ((axeCore as unknown) as AxeWithAudit)._audit.data.failureSummaries[ | ||
| key | ||
| ].failureMessage(node[key].map(check => check.message || '')); | ||
| const message: string = ( |
There was a problem hiding this comment.
sorry for this noise; these changes are from the formatter
| @@ -286,6 +289,8 @@ function addComponent(component: any): void { | |||
| const reactInstanceDebugID = reactInstance._debugID; | |||
| const reactFiberInstance = component._reactInternalFiber || {}; | |||
| const reactFiberInstanceDebugID = reactFiberInstance._debugID; | |||
| const reactInternals = component._reactInternals || {}; | |||
There was a problem hiding this comment.
the rest of this code is the fix
|
Hey @seanparmelee, Apologies for the delay, we will review this today. |
|
An update on this. We've been able to confirm this fixes the problem, but are trying to figure out the best way to add a test so we can catch something like this in the future. |
|
Hey @seanparmelee, We will need to update our entire test suite, so no test required at the moment. We have confirmed your fix works as intended. Thank you for contributing |
|
The security review check has been running for days, what is the normal timeline for that succeeding? |
|
We have had to cherry-pick this PR to get CI running. Will be merged shortly. Reference: #437 |
|
Thanks @Zidious, so it sounds like we can close this one out then? |
|
Also, while I was investigating all of this, I noticed that the |
|
Resolved in #455 |
I was troubleshooting an issue where subsequent renders were not being checked for issues. Turns out the issue is due to the
_reactInternalFiberfield being renamed to_reactInternalsin v17: facebook/react#18377.This PR adds a fallback for
_reactInternals. You can see this issue in action if you bump the shadow-dom example to React 17, run it, and then click on one of the services. You will not see the additional log statement in the console after clicking the service. I expect this to also fix #415.Let me know if/how you'd like tests for this change. I'm guessing you'd probably prefer the shadow-dom example to remain at React 16, but maybe we could add another integration test that uses the NextJS example, which is already using React 17?