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

External objects should not be modified #7655

Closed
micnic opened this issue Jul 11, 2016 · 8 comments
Closed

External objects should not be modified #7655

micnic opened this issue Jul 11, 2016 · 8 comments
Labels
discuss Issues opened for discussions and feedbacks. fs Issues and PRs related to the fs subsystem / file system.

Comments

@micnic
Copy link
Contributor

micnic commented Jul 11, 2016

Hey, I was playing around with Object.seal() and Object.freeze() and discovered that internally Node is trying to modify my objects and I got the following in fs.watch:

TypeError: Can't add property recursive, object is not extensible

In my opinion external objects should not be modified by Node, as I observed sometimes this rule is respected, sometimes - only partially (in case of fs.appendFile).

Do we have anywhere this rule specified?

cc @nodejs/collaborators

@micnic micnic added discuss Issues opened for discussions and feedbacks. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 11, 2016
@targos
Copy link
Member

targos commented Jul 11, 2016

I don't know of any written rule but I agree that we should not modify user-supplied objects.

@thefourtheye
Copy link
Contributor

We can solve this problem for options in #7165 by making a copy.

@targos
Copy link
Member

targos commented Jul 11, 2016

We can solve this problem for options in #7165 by making a copy.

Another way is to do options = Object.create(options);

@micnic
Copy link
Contributor Author

micnic commented Jul 11, 2016

@thefourtheye yes, it would be a good place to add a solution for the fs module, I could not find yet this issue for other core modules.

util._extend() should be used in this case I guess

@thefourtheye
Copy link
Contributor

@micnic I used Object.assign and introduced a commit in #7165.

@micnic
Copy link
Contributor Author

micnic commented Jul 12, 2016

@thefourtheye as I know from #7208 and #7255 Object.assign() performs slower than util._extend() and it was decided to use it until Object.assign() will have a better performance

@micnic micnic added fs Issues and PRs related to the fs subsystem / file system. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 12, 2016
@thefourtheye
Copy link
Contributor

@micnic Right. I use util._extend now.

thefourtheye added a commit to thefourtheye/io.js that referenced this issue Oct 9, 2016
This patch makes a copy of the `options` object before the fs module
functions alter it.

PR-URL: nodejs#7831
Fixes: nodejs#7655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Nicu Micleușanu <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@thefourtheye
Copy link
Contributor

Fixed in 7542bdd

jasnell pushed a commit that referenced this issue Oct 10, 2016
This patch makes a copy of the `options` object before the fs module
functions alter it.

PR-URL: #7831
Fixes: #7655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Nicu Micleușanu <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
kevinoid pushed a commit to kevinoid/fs-file-sync-fd that referenced this issue Dec 13, 2016
This patch makes a copy of the `options` object before the fs module
functions alter it.

PR-URL: nodejs/node#7831
Fixes: nodejs/node#7655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Nicu Micleușanu <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
kevinoid pushed a commit to kevinoid/fs-file-sync-fd that referenced this issue Dec 13, 2016
This patch makes a copy of the `options` object before the fs module
functions alter it.

PR-URL: nodejs/node#7831
Fixes: nodejs/node#7655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Nicu Micleușanu <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
kevinoid pushed a commit to kevinoid/fs-file-sync-fd that referenced this issue Dec 13, 2016
This patch makes a copy of the `options` object before the fs module
functions alter it.

PR-URL: nodejs/node#7831
Fixes: nodejs/node#7655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Nicu Micleușanu <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

No branches or pull requests

3 participants