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

Hoek.clone() issues #228

Closed
kanongil opened this issue Jan 30, 2018 · 9 comments
Closed

Hoek.clone() issues #228

kanongil opened this issue Jan 30, 2018 · 9 comments
Labels
bug
Milestone

Comments

@kanongil
Copy link
Member

@kanongil kanongil commented Jan 30, 2018

I happened to look into the clone() implementation, and found several critical issues in my review:

  1. Cloned Map / Set values are completely broken.
  2. Does not clone Symbol() properties on object. Can be found using Object.getOwnPropertySymbols().
  3. Non-enumerable properties are enumerable on cloned object.
  4. Cloned Promise values are broken.
@kanongil kanongil added the bug label Jan 30, 2018
@kanongil

This comment has been minimized.

Copy link
Member Author

@kanongil kanongil commented Jan 30, 2018

Another type that could use special handling is Error.

I know that due to lazy evaluation, reading the stack property is extremely slow in V8, and should be avoided. I would probably cheat a little here, and install a getter instead, which can then retrieve the original stack value.

Object.defineProperty(newObj, 'stack', {
    configurable: true,
    get() {

        return obj.stack;
    },
    set(value) {

        Object.defineProperty(newObj, 'stack', {
            configurable: true,
            writable: true,
            value
        });
    }
});

See https://bugs.chromium.org/p/v8/issues/detail?id=5962 for a bit of background.

@nlf

This comment has been minimized.

Copy link
Member

@nlf nlf commented Jan 30, 2018

yup, clone hasn't been updated for newer types (even as new as Promise)

good catch on cloned non-enumerable properties becoming enumerable though!

@nlf nlf added the help wanted label Jan 30, 2018
@robmcguinness

This comment has been minimized.

Copy link

@robmcguinness robmcguinness commented Mar 25, 2018

For completeness: Hoek.applyToDefaults also doesn't take into account Symbol properties of an object.

@SimonSchick

This comment has been minimized.

Copy link

@SimonSchick SimonSchick commented Aug 19, 2018

It's also problematic for complex types such as sockets, I ran into this this catbox-redis just now.

@kanongil

This comment has been minimized.

Copy link
Member Author

@kanongil kanongil commented Aug 20, 2018

@SimonSchick Yeah, any object with hidden state won't work unless explicitly added. It's only designed to work for "simple" objects.

@hueniverse hueniverse removed the help wanted label Nov 1, 2018
@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Nov 11, 2018

@kanongil any plans on fixing this?

@kanongil

This comment has been minimized.

Copy link
Member Author

@kanongil kanongil commented Nov 24, 2018

The enumerable issue is fixed with 6f02157.

@kanongil

This comment has been minimized.

Copy link
Member Author

@kanongil kanongil commented Nov 26, 2018

The Symbol issue should be fixed with #281.

@shijiebei2014

This comment has been minimized.

Copy link

@shijiebei2014 shijiebei2014 commented Apr 7, 2019

const Hoek = require('hoek')
var map = new Map([
['1', new Object()]
])
console.log(Hoek.clone(map))

The result is : Map {}

@hueniverse hueniverse added this to the 6.2.3 milestone May 17, 2019
@hueniverse hueniverse closed this May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.