Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc.parent() and doc.ownerDocument() consistency #10884

Closed
chumager opened this issue Oct 14, 2021 · 3 comments · Fixed by #11005
Closed

doc.parent() and doc.ownerDocument() consistency #10884

chumager opened this issue Oct 14, 2021 · 3 comments · Fixed by #11005
Assignees
Labels
new feature This change adds new functionality, like a new method or class
Milestone

Comments

@chumager
Copy link

Do you want to request a feature or report a bug?
feature
What is the current behavior?
doc.parent() always exists, but doc.ownerDocument() don't.

If a call doc.parent() from a root document it returns undefined.

doc.ownerDocument() isn't in the API documentation.

IMHO doc.parent() and doc.ownerDocument() should return this if is a root document, and for consistency, both functions should always exists.

If I made a plugin who needs some data from ownerDocument, I've to do things like:

const {field} = this.ownerDocument?.() || this;

And I've a lot of definitions like this.

For example in vue.js this.$root is always defined even though this is the root.

This is useful in case you want to know if it's not a subDocument:

if(this !== this.ownerDocument()){
  //some stuff
}

this consistency let you be more agnostic about the data structure.

What is the expected behavior?
First of all it would be good if those functions were in the API docs.
Second, I think it's a good idea to have the functions always defined.
And last, those functions should return this in case of a root document.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.
mongoose: 6.0.9
node: 16.11.0
mongodb attlas: 4.4.9

@vkarpov15 vkarpov15 added this to the 6.0.12 milestone Oct 14, 2021
@vkarpov15
Copy link
Collaborator

Makes sense. Right now we sometimes use if (this.ownerDocument == null) in our code to check if we're looking at a top-level document, but I can see the argument for consistency. We'll fix this.

@chumager
Copy link
Author

IMHO besides this is a good solution for me, it could be a breaking change for developers.

I while ago, in between patches, doc.parent() disappears for root documents and I'd to change all my related code (a lot), then it changed back, and I'd to change all again.

If someone uses if (this.ownerDocument == null) it'll break his code.

@IslandRhythms IslandRhythms added the new feature This change adds new functionality, like a new method or class label Oct 15, 2021
@vkarpov15 vkarpov15 modified the milestones: 6.0.12, 6.1.0 Oct 19, 2021
@vkarpov15
Copy link
Collaborator

In that case maybe we move it to 6.1.0. We added parent() in a minor release, so wouldn't be too far off to use ownerDocument.

@IslandRhythms IslandRhythms linked a pull request Nov 23, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature This change adds new functionality, like a new method or class
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants