fix(document): make array getters avoid unintentionally modifying array, defer getters until index access instead #13774
Conversation
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
…ay, defer getters until index access instead Fix #13748
vkarpov15 added a commit that referenced this pull request Aug 28, 2023
vkarpov15 added a commit that referenced this pull request Aug 29, 2023
fix(document): avoid double-calling array getters when using `.get('arr.0')` 
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Summary
#13748 points out that accessing
user.hobbiesactually modifies thehobbiesarray if there's a getter on elements of thehobbiesarray. That is a problem. To fix that, I moved the logic for applying getters fromSchemaArray.prototype.applyGetters(), which is also called when you accessuser.hobbies, and into the top-levelapplyGetters()function, which is only called bytoObject()andtoJSON().As a side effect, in order to avoid breaking Schema UUID tests, we instead need to apply getters on index access. So
user.hobbies[0]will fire getters onhobbies.0, but not modifyhobbies. That's what the changes totypes/array/index.jsare for.Still to investigate: it looks like
oneUser.get('hobbies.0')double calls getters with this PR. Worth checking before merging.Also need to think about whether to merge this in 7.5, or wait for 8.0. The
types/array/index.jschanges seem a bit too heavy for a patch release.Examples