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

Add use of prototype._clone() if found to clone an object. + test #93

Closed
wants to merge 5 commits into from

Conversation

btsimonh
Copy link

@btsimonh btsimonh commented Mar 17, 2018

This commit adds a feature by which if an object has a prototype method _clone(), this method is used to clone that object and all descendants.
Rational:
Currently cloning of an object is done with Object.create().
This does not work for certain objects (specifically node-opencv mat and opencv4nodejs cv.Mat objects).
For any object which does not clone correctly, a user can add a _clone() function and node-clone will then clone to the user's required specification.
Other examples where this is useful:
The _clone() function can do anything the user wants with an object. For example, it could mearly return the original object, or return the original object with a reference count incremented. Or as in the test, the clone could be marked as a clone...

questions: is _clone() as the function name sufficiently 'conflict free'?

  • update: modified to allow an object to have a plain function _clone() attached - in this way behaviour for an individual object can be modified as well. Obviously, if a prototype._clone() method is not added, the resulting object may no longer be cloneable.

…only in the prototype. This allows a user to customise a Specific object to behave differently to the prototype method.
@pvorb
Copy link
Owner

pvorb commented Mar 21, 2018

I dislike the idea of having cloned objects define a prototype function. It seems to me that this should be part of it's own library, that you could use if clone doesn't work with an object.

@btsimonh
Copy link
Author

the problem is that the clone library is used by another project (Node-Red), which uses it to clone generic messages; so we would prefer not to change the library for another. We want to send objects down these messages, and have no control over the cloning process (i.e. we can;t choose to clone in a different way). We have spent days over a period of months looking at mechanisms to make clone work for us, but all attempts have failed.
So what we are proposing is that we add a 'getout' clause, allowing the objects themselves to determine the cloning mechanism should the object/user so desire. If we can get this into your library, getting the NR team to update will not be an issue (but getting a PR in for NR to change the library for another would take another 6 months :) ).
Added to readme as requested.

@pvorb
Copy link
Owner

pvorb commented Mar 22, 2018

clone is used by almost 2000 other projects. I can't make sure that this change won't be breaking other people's code.

Native objects always have been problematic. Why don't you just wrap your call to clone and invoke the _clone() method instead, if it is available? This way, you don't have to fork clone or use another library. What do you think?

@btsimonh
Copy link
Author

Hi Paul,

the root object being cloned is a message structure, which users would expect to be able to place a native object at any level in the structure, and the project (Node-Red) is also risk-averse. The mod is low risk, except for the name of the function (_clone()), so could we compromise on a more complex name; less risk of conflict? eg ___cloneObject() (3 underscores)?
I see this as a big advance in clone technology; enabling customization without modification. e.g. recent additions to the your project involve clone of specific object types; nativeMap, nativeSet, nativePromise; each of these get special treatment. The mod allows any user to enable clone of their object type, without a mod to clone itself, and even to do more; e.g. to make an object not clone, but instead reference ( myobject._clone= function() {return this); ), if so desired (i.e. it enables Control, at an object level).
And you would no longer need to worry about native objects (or request for support for objects you don't want to); the objects would have a way to look after themselves - giving you the 'getout' clause :).

thoughts?

s

@bartbutenaers
Copy link

Hi Paul,

I am contributing together with Simon on Node-Red, so thank you very much for having this discussion with us!!!!

Some context

In Node-Red messages are send between blocks, that are wired together:
image

In the first block we create an OpenCv image (i.e. Javascript wrapper around native memory), which is send to the next blocks in the payload field of a message object. The original message object is send to the first wire (1), but a clone of the message is send to the next wire (2). To clone the message, Node-Red calls your module and we have NO way to work around it ;-(

Requirement

The only way to create a valid clone of the Javascript wrapper, is by using the API of the OpenCv wrapper:
someWrapper.copy()

So we try to find some way to customize the cloning process, by calling this code snippet ...

Failed experiments

We are aware that your project is very popular, so we tried to find a way to call that code snippet (without pull-request) but all our experiments have failed:

  • Constructor: the Object.create doesn't call the constructor
  • Proxy object: the Proxy getter is called by your module, but the clone is a normal object (instead of a Proxy object). Your module calls the getter only to get the data.
  • Getters and setters: Same result as with a Proxy. When we specify getters and setters in the message object, the cloned object doesn't contain the getters and setters anymore.
  • Cloneable: Unfortunately Javascript doesn't seem to have a Cloneable interface (like Java, C# ...). So therefore Simon created this pull request to have at least something alike ...

But perhaps you have better ideas to allow us to customize the cloning. I.e. some way that doesn't break other projects, and could be helpfull for other users also.

Thanks in advance,
Bart

@pvorb
Copy link
Owner

pvorb commented Mar 22, 2018

Thank you, Simon and Bart, for providing some context.

Can you detect whether you are cloning such a wrapper object around an OpenCV image/object or is it deeply nested in an object structure that you clone all at once?

Can you give me a link to the code we're talking about? Probably I can think of a solution when I have a specific example.

@btsimonh
Copy link
Author

Dear Paul,

I have prepared a Simple repo (based on an example from the guy who make opencv4nodejs) here:

https://github.com/btsimonh/node-addon-tutorial

Pull it down,
cd to the VectorExample folder,
do 'npm install'
look at index.js
run with 'npm start' or 'node index.js'

This reproduces our issue in the simplest form. The native Vector object seems to clone, but as soon as you try to use 'add', you either get 'TypeError: Illegal invocation' (if you try to run the add function from the clone), or 'Vector::Add - expected argument to be instance of Vector' (if you try to pass a clone into .add on a real Vector).
Note: I have only built on Windows.

Additional info on the Actual objects we are trying to clone:
So the object in question is Mat from opencv4nodejs
https://github.com/justadudewhohacks/opencv4nodejs/blob/master/cc/core/Mat.h
installing opencv4nodejs takes 4Gbytes and 2 hours, so we won't ask you to do that to try :).
We are then using that in Node-Red, and the Mat object could be at any depth in the message structure. The specific use of Clone in Node-Red is in function cloneMessage(msg) in

https://github.com/node-red/node-red/blob/716bca211b7105d2b3e13a1c3802b7f5e572620b/red/runtime/util.js

Note that Node-Red already has one issue where 'req' and 'res' in the root of the message need to be referenced, not copied (these are http request and results). Our data would normally be called 'payload' and be in the root of message, but could be msg.payload.img, etc. and there could be multiple in one message.

At the end of the index.js in the Simple Repo above, I've made an example Node-Red message as illustration.

thanks for your consideration,
Simon

@btsimonh
Copy link
Author

Added to the end of the Simple Repo a http response clone test - the thing that Node-Red manually avoids.

@btsimonh
Copy link
Author

Hi Paul,

what are your thoughts?

Simon

@pvorb
Copy link
Owner

pvorb commented Apr 26, 2018

Sorry, was a bit overwhelmed by that header file and regretted that I asked in the first place.

So, if I try to run npm install in VectorExample, I just get:

$ npm install

> node-addon-tutorial-vector-example@1.0.0 install C:\Users\Paul\workspace\playground\node-addon-tutorial\VectorExample
> node-gyp rebuild


C:\Users\Paul\workspace\playground\node-addon-tutorial\VectorExample>if not defined npm_config_node_gyp (node "C:\Users\Paul\AppData\Roaming\npm\node_modules\npm\bin\node-gyp-bin\\..\..\node_modules\node-gyp\bin\node-gyp.js" rebuild )  else (node "" rebuild )
Traceback (most recent call last):
  File "C:\Users\Paul\AppData\Roaming\npm\node_modules\npm\node_modules\node-gyp\gyp\gyp_main.py", line 13, in <module>
    import gyp
ImportError: No module named gyp
gyp ERR! configure error
gyp ERR! stack Error: `gyp` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onCpExit (C:\Users\Paul\AppData\Roaming\npm\node_modules\npm\node_modules\node-gyp\lib\configure.js:305:16)
gyp ERR! stack     at emitTwo (events.js:87:13)
gyp ERR! stack     at ChildProcess.emit (events.js:172:7)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)
gyp ERR! System Windows_NT 6.1.7601
gyp ERR! command "C:\\Program Files\\NodeJS\\node.exe" "C:\\Users\\Paul\\AppData\\Roaming\\npm\\node_modules\\npm\\node_modules\\node-gyp\\bin\\node-gyp.js" "rebuild"
gyp ERR! cwd C:\Users\Paul\workspace\playground\node-addon-tutorial\VectorExample
gyp ERR! node -v v4.4.5
gyp ERR! node-gyp -v v3.3.1
gyp ERR! not ok

npm WARN node-addon-tutorial-vector-example@1.0.0 No description
npm ERR! Windows_NT 6.1.7601
npm ERR! argv "C:\\Program Files\\NodeJS\\node.exe" "C:\\Users\\Paul\\AppData\\Roaming\\npm\\node_modules\\npm\\bin\\npm-cli.js" "install" "."
npm ERR! node v4.4.5
npm ERR! npm  v3.9.3
npm ERR! code ELIFECYCLE
npm ERR! node-addon-tutorial-vector-example@1.0.0 install: `node-gyp rebuild`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the node-addon-tutorial-vector-example@1.0.0 install script 'node-gyp rebuild'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the node-addon-tutorial-vector-example package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     node-gyp rebuild
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs node-addon-tutorial-vector-example
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls node-addon-tutorial-vector-example
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     C:\Users\Paul\workspace\playground\node-addon-tutorial\VectorExample\npm-debug.log

@btsimonh
Copy link
Author

hi paul,

hmm... some time since I did this.
I'll take a look, but not a specialist in build errors....
you have windows build env?

s

@btsimonh
Copy link
Author

Hi Paul,
seems I've been a little lax on this one. What exact environment are you running; maybe I can send you a binary image so you don't need to build?

I still believe this idea may help in a number of your outstanding issues; e.g. it would enable user workarounds for many of the issues in your issue list, giving them the choice of customising the clone method for chosen objects, even if they have no control over the code where clone is called...
#92
#82
#80
could all be worked around by the user...

best regards,

Simon

@btsimonh
Copy link
Author

replacing with a simpler modification. Watch this space :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants