fix: fieldDropdown.getText works in node#9048
fix: fieldDropdown.getText works in node#9048maribethb merged 2 commits intoRaspberryPiFoundation:rc/v12.0.0from
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Thanks @maribethb! Approving to unblock, but had some thoughts (most of which likely don't need to happen now for v12).
|
|
||
| assert.deepEqual(jsonAfter, json); | ||
| }); | ||
| test('Dropdown getText works with no HTMLElement defined', function () { |
There was a problem hiding this comment.
Just to check: this test fails without the fix in place, right?
| "you're using HTMLElement dropdown options in node, ensure you're " + | ||
| 'using jsdom-global or similar.', | ||
| ); | ||
| return null; |
There was a problem hiding this comment.
Is it worth verifying the console warning & null case in tests?
There was a problem hiding this comment.
No, because in order to get here you'd have to pass something that is an HTMElement type but then don't have HTMLElement defined. It would be really annoying to set up in tests and is a scenario that is unlikely to happen at all. But I had to return null to satisfy the ts compiler and the warning helps figure out why that is, in the off chance it ever somehow does happen.
3 participants
The basics
The details
Resolves
Fixes #9035
Proposed Changes
Checks that
HTMLElementis defined before trying to use itReason for Changes
Dropdown fields should work in node
Test Coverage
Added a node test to make sure this change fixes the CI problems we saw in samples
Documentation
I logged a warning for the case that would hit if:
FieldDropdownwithHTMLElementoptionsHTMLElement, and callgetText.Honestly the chances anyone does this and doesn't run into a number of other issues is relatively small, but hopefully the warning message helps them out if so.
If someone is using node with just regular text options, they don't need to supply an implementation of
HTMLElementand won't see the warning, so this doesn't affect the usual case (and before v12, the only case, as it wasn't possible to use element options).Additional Information