fix: make behavior consistent for undefined nested fields and newer TS targets#15
Closed
dliv wants to merge 3 commits into
Closed
fix: make behavior consistent for undefined nested fields and newer TS targets#15dliv wants to merge 3 commits into
dliv wants to merge 3 commits into
Conversation
dliv
commented
Apr 16, 2025
Comment on lines
+118
to
+130
| // remove block in the future | ||
| // see: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#the-usedefineforclassfields-flag-and-the-declare-property-modifier | ||
| { | ||
| const recentTsTarget = Object.keys(testingClass1).includes('nestedItem'); | ||
| if (!recentTsTarget) { | ||
| console.debug('older TS target'); | ||
| // make test object look like what newer TS targets generate | ||
| Object.assign(testingClass1, { | ||
| nestedItem: undefined, | ||
| nestedItems: undefined, | ||
| }); | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
This is definitely a hack. I thought about enabling the flag mentioned in those docs but it would probably be cleaner just to update the TS target. I'm not sure how extensive those changes would end up being or if you're interested in doing that. I might be able to help if you were.
dliv
commented
Apr 16, 2025
|
|
||
| if (prop.isNested) { | ||
| const value = entity[prop.options.entityPropName]; | ||
| if (prop.isNested && value !== undefined && value !== null) { |
Contributor
Author
There was a problem hiding this comment.
The value !== null is scope creep not related to the TS config but it seems like a good / defensive idea. I'm fine backing that out.
Owner
|
Reimplemted here #16 |
Owner
Contributor
Author
|
That works for us. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We encountered runtime errors when omitting a nested field. I'm not sure if the library intends to support optional nested fields or not.
If it does, the current code can throw if the consuming application's tsconfig has a newer
target. The way uninitialized fields are treated is changing: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#the-usedefineforclassfields-flag-and-the-declare-property-modifier .The relevant lines are
elasticsearch-orm/src/utils/EntityTransformer.ts
Lines 51 to 59 in f017a25
That config change affects whether an
undefinedvalue is enumerated on line 51, and then recursed on line 58, causing an NPE-like error on line 53.To verify, you can change the target in your tsconfig to
ES2022and remove the application code tweak in this PR and the new unit test will fail. I'm not sure the fix in this PR is ideal, feel free to change / suggest-changes, but the new unit test demonstrates our current usage / config.