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

fix Node.js 17.5 compatibility #11381

Merged
merged 3 commits into from
Feb 13, 2022
Merged

Conversation

benjamingr
Copy link
Contributor

@benjamingr benjamingr commented Feb 11, 2022

The tc39 async iterator helpers spec adds .map to async iterators and Node added support for that.

As part of spec compliance Node uses defineProperty to add these properties.

This PR fixes setting .map in Mongoose over the property.

As a side note: it might make sense for Mongoose to deprecate .map since it's mutative which makes it named the same but with a different behavior from the tc39 iterator helpers proposal .map

Fixes: #11377

The tc39 async iterator helpers spec adds `.map` to async iterators and Node added support for that.

As part of spec compliance Node uses defineProperty to add these properties.

This PR fixes setting `.map` in Mongoose over the property.

As a side note: it might make sense for Mongoose to deprecate `.map` since it's mutative which makes it named the same but with a different behavior from the tc39 iterator helpers proposal `.map`
@nateevans
Copy link

FYI - this change works, I used it as a patch, but the same issue exists here
https://github.com/Automattic/mongoose/blob/master/lib/cursor/AggregationCursor.js#L140

@benjamingr
Copy link
Contributor Author

Thanks @nateevans anywhere else?

@nateevans
Copy link

I'll let you know, actively going through it now

@benjamingr
Copy link
Contributor Author

Feel free to push to this branch or PR it if pushing directly doesn't work

@nateevans
Copy link

@benjamingr that seems to be the 2 files with the issue

@Uzlopak
Copy link
Collaborator

Uzlopak commented Feb 12, 2022

@benjamingr

Can you please run the lint fixer over your code?

npm run lint -- --fix

@benjamingr
Copy link
Contributor Author

@Uzlopak sure done

@Slayer95
Copy link

Here it should be enumerable and configurable, shouldn't it?

@benjamingr
Copy link
Contributor Author

@Slayer95 probably not unless you think people are expecting it to show up on for… in loops

@Slayer95
Copy link

I mean, the current implementation does that, so the fix should keep it. Otherwise, it might be the better plan to update all methods to your approach.

Just pointing that out. Maintainer shall know better.

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍 I'll make these properties enumerable and writable in master, if possible.

@vkarpov15 vkarpov15 added this to the 6.2.2 milestone Feb 13, 2022
@vkarpov15 vkarpov15 merged commit e5f13a8 into Automattic:master Feb 13, 2022
vkarpov15 added a commit that referenced this pull request Feb 13, 2022
@benjamingr benjamingr deleted the patch-1 branch February 13, 2022 22:16
@Eunit99
Copy link

Eunit99 commented May 5, 2022

This fix does not resolve #11377 as claimed. I had to downgrade my NodeJS to version 12.x. to resolve #11377. More information on how I did that is found in this tutorial.

@Uzlopak
Copy link
Collaborator

Uzlopak commented May 5, 2022

Your tutorial is missing the point. Downgrading to node 12 is like saying: Hey your Tesla needs Electricity, but to still use gasoline use a Ford Model-T. Despite the fact you could use a modern car, which uses gasoline.

Also our CI/CD Pipelines is testing for node18, which actually would fail if that issue was still existing. But the patch by @benjamingr is solving that issue also for node18.

So to solve that issue, you would need to upgrade mongoose to latest version OR use any version of node 17 except 17.5.0 and node 18 and above. A downgrade to node 12 is not necessary.

And mongoose 6.3.2 does not has that issue, which you describe.

@Automattic Automattic locked and limited conversation to collaborators May 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
7 participants