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

Regression in Node 17.5, Assigning a function to prototype of an Object results in a TypeError: Cannot assign to read only property 'x' of object 'y' at Object.<anonymous> #41926

Closed
Uzlopak opened this issue Feb 11, 2022 · 21 comments · Fixed by #41931
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@Uzlopak
Copy link
Contributor

Uzlopak commented Feb 11, 2022

Version

17.5

Platform

Linux aras-Lenovo-Legion-5-17ARH05H 5.13.0-21-generic #21-Ubuntu SMP Tue Oct 19 08:59:28 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

We maintain the mongoose-project. Devs reported that node 17.5 is throwing a TypeError when using the package.
I tested it with node 17.5 and run the unit tests, and could reproduce the Error. When I test with node 17.4, there is no TypeError.
I checked the Changelog and there is no remark regarding a breaking change, nor does the changes implicate a breaking change in nodes behaviour.

So I assume that there is a Regression in node 17.5.

Maybe something with the attribute "map" is special?

Also is reported that adding "engines": { "node": ">=12.0.0 <17.5.0" }, to package.json fixes the issue.
Is there some nodejs quirk mode? Do we have to change something in our codebase to not raise this Error?

Just checkout mongoose-project, install dependencies, and run the tests under node 17.5

See also:
Automattic/mongoose#11377

How often does it reproduce? Is there a required condition?

Happens always.

What is the expected behavior?

Should not throw a TypeError like it did not throw in node <= 17.4 and unit tests should pass as usual.

What do you see instead?

When running the tests we get:

TypeError: Cannot assign to read only property 'map' of object '#<QueryCursor>'
    at Object.<anonymous> (/home/aras/Workspace/mongoose/lib/cursor/QueryCursor.js:144:27)
    at Module._compile (node:internal/modules/cjs/loader:1097:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1151:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:999:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/home/aras/Workspace/mongoose/lib/query.js:12:21)
    at Module._compile (node:internal/modules/cjs/loader:1097:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1151:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:999:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/home/aras/Workspace/mongoose/lib/index.js:18:15)
    at Module._compile (node:internal/modules/cjs/loader:1097:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1151:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:999:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/home/aras/Workspace/mongoose/index.js:9:18)
    at Module._compile (node:internal/modules/cjs/loader:1097:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1151:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:999:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/home/aras/Workspace/mongoose/test/common.js:7:18)
    at Module._compile (node:internal/modules/cjs/loader:1097:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1151:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:999:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/home/aras/Workspace/mongoose/test/aggregate.test.js:7:15)
    at Module._compile (node:internal/modules/cjs/loader:1097:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1151:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:168:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:197:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:337:24)
    at async importModuleDynamicallyWrapper (node:internal/vm/module:437:15)
    at async formattedImport (/home/aras/Workspace/mongoose/node_modules/mocha/lib/nodejs/esm-utils.js:7:14)
    at async Object.exports.requireOrImport (/home/aras/Workspace/mongoose/node_modules/mocha/lib/nodejs/esm-utils.js:48:32)
    at async Object.exports.loadFilesAsync (/home/aras/Workspace/mongoose/node_modules/mocha/lib/nodejs/esm-utils.js:103:20)
    at async singleRun (/home/aras/Workspace/mongoose/node_modules/mocha/lib/cli/run-helpers.js:125:3)
    at async Object.exports.handler (/home/aras/Workspace/mongoose/node_modules/mocha/lib/cli/run.js:374:5)

Additional information

No response

@Slayer95
Copy link

According to your docs,

  • A QueryCursor exposes a Streams3 interface, as well as a .next() function.

That conflicts with the fact that map() is now an iterator helper method for Node.js streams.

But... why was that method added to Node.js core as non-writable ??

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Feb 11, 2022

Maybe core-devs want to ensure that map is not overwritten for performance reasons?

@Trott
Copy link
Member

Trott commented Feb 11, 2022

@nodejs/streams

@mcollina
Copy link
Member

cc @benjamingr that was my fear this could happen. Almost every time we added properties to the streams this happened.

Either we:

  1. make all the new properties writable (preferred)
  2. revert them in v17.x and ship them in v18

@ronag
Copy link
Member

ronag commented Feb 11, 2022

maybe 1. with a warning if one does overwrite it?

@benjamingr
Copy link
Member

Making the new properties writable is a spec violation. I actually tested this with mongoose since they do weird things with streams and unfortunately missed this/didn't run into it.

I'll see if I can open a PR to fix this in mongoose too.

@benjamingr
Copy link
Member

Wow mongoose's map is mutative that's so weird... hmm. Let me think about this a bit.

@benjamingr
Copy link
Member

Opened a Mongoose PR Automattic/mongoose#11381

@benjamingr
Copy link
Member

First of all to the users who ran into this @Slayer95 @Uzlopak I am sorry! It was obviously not my intention to cause this breakage, I actually tested with Mongoose but probably not after the defineProperty change.

But... why was that method added to Node.js core as non-writable ??

This is required for spec compliance and to pass a language-proposal test. Though note you can still override non-writable properties as shown in the PR above.

@benjamingr
Copy link
Member

revert them in v17.x and ship them in v18

This is probably also fine, or make them writable in v17 but not v18

@Lysak
Copy link

Lysak commented Feb 11, 2022

To solve this error need to install nvm (Node Version Manager) and run nvm install --lts, nvm alias default lts/* and nvm use --lts

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Feb 11, 2022

First of all, thank you for your quick reaction. For me personally nothing changes, as I am not using bleeding edge. But I guess those who want to use bleeding edge are effected ;). But this is tbh the risk someone takes if he/she uses bleeding edge.

@benjamingr
Thank you for the PR in mongoose. Probably we should like you write in the PR, not overwrite map. Maybe this could also mean a perf. degredation, or maybe we have already perf degredations already... But this is something in mongoose-land and not in node-land.

@Lysak
I dont think that it makes sense to install LTS-version if we have a breaking change in node 17 and somebody is using node 17 on purpose.

@benjamingr
Copy link
Member

Thank you for the PR in mongoose. Probably we should like you write in the PR, not overwrite map. Maybe this could also mean a perf. degredation, or maybe we have already perf degredations already... But this is something in mongoose-land and not in node-land.

For what it's worth Node.js takes the ecosystem very seriously and a breakage or performance regression in Mongoose because of a change in Node.js is definitely something we want to be aware of and to address. This is double true for breakage that causes programs to not work and even more so when the change isn't communicated in advance and labeled as semver-major.

@benjamingr
Copy link
Member

A fix landed in master and will be released with the next Node.js release - sorry for the mess!

@overmars86
Copy link

So shall we wait for the next release or find a temp. Solution? I need to fix it ASAP, please.

@benjamingr
Copy link
Member

@overmars86 you can either run a version you build from master (of either mongoose or node) or use a different (lts) version of Node in the meantime

greschner added a commit to greschner/cpee-worklist that referenced this issue Feb 14, 2022
@SergioArrighi
Copy link

Hello, is there any ETA for the new release?
Thanks and best regards

bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
PR-URL: nodejs#41931
Fixes: nodejs#41926
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
PR-URL: nodejs#41931
Fixes: nodejs#41926
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
bengl pushed a commit that referenced this issue Feb 22, 2022
PR-URL: #41931
Fixes: #41926
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@lolo-io
Copy link

lolo-io commented Feb 23, 2022

Latest version 17.6.0 with the fix (#41931) is out : https://github.com/nodejs/node/releases/tag/v17.6.0
Tested it and the error is gone.

Not available in docker yet as :latest image still points to 1.17.5 but it should be available very soon.

@rochdev
Copy link
Contributor

rochdev commented Feb 25, 2022

@targos targos reopened this Feb 25, 2022
@targos targos added the stream Issues and PRs related to the stream subsystem. label Feb 25, 2022
@aduh95
Copy link
Contributor

aduh95 commented Feb 25, 2022

TypeError: Cannot assign to read only property 'forEach' of object '#'

Looks like we need to apply the same fix for all the new methods.

@benjamingr
Copy link
Member

Ok, there is already a merged pr that did this (the one porting more test262 tests)so I suggest we keep this open until the next release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.