Fix e.type is not iterable error #107
Conversation
ngfreiter
requested review from
cmoesel,
mint-thompson,
kjmahalingam and
jafeltra
| @@ -472,7 +472,7 @@ export class StructureDefinition { | |||
| let matchingType: ElementDefinitionType; | |||
| const matchingXElements = elements.filter(e => { | |||
| if (e.path.endsWith('[x]')) { | |||
| for (const t of e.type) { | |||
| for (const t of e.type ?? []) { | |||
There was a problem hiding this comment.
Good catch! But I notice that you had to do this here, as well as in another location. I imagine we might have to iterate over type in other places in the future, as well.
Is there a reason that we can't just modify the type property itself to be set to an empty array if it isn't otherwise set? That might prevent us from having to catch cases like this in the future.
There was a problem hiding this comment.
Yeah that is a good point. I am not sure how to go about setting type though, because we don't want to output JSON with an empty array for type.
There was a problem hiding this comment.
My one concern there is that null types are usually something we inherit from the parent. So when we serialize back out to JSON, we'd have to make sure that we don't change what was a null type to an empty type ([]) instead. Might have to put special logic in the serialized to make that happen.
Another possible approach would be to introduce a nullSafeType getter or function and try to always use that instead. But then I guess we need to ask if that's any easier than remembering to do ?? [] as above.
There was a problem hiding this comment.
Given the issue Chris raises, I would consider rescinding my suggestion. If there's a distinct difference between null and empty for serialization, then getting around that would arguably be more unclear than what we are doing.
|
Do we want a test for this now that we can ensure it functions correctly? |
|
All checks pass for me, and the changes generally seem to work, though I still would like to see if there's anything to address with my comments above. |
|
Ok @kjmahalingam I added a test that would fail without this fix, so it shows this is working correctly. Is there a consensus with you and @cmoesel on if I should add extra handling to the |
I noticed when running SUSHI that an error would pop up sometimes saying "e.type is not iterable". On an ElementDefinition, type is technically not required, and there were some places we were assuming type would exist as an array. This PR fixes the places where we made those assumptions so that the error doesn't happen.