toObjectMethod being called by different argument types

[anthify]

Version

EDIT: 2.4.0 2.6.0

Details

Firstly, sorry to go off the issue format. I'm working on a biggish project which extends quite a few different classes so it might be a problem of my own making.

The _toObjectMethod is called recursively if clipPath resolves to true, but instead of methodName being passed which (afaik) should be a string, and in this instance should have the value"toObject". But as you can see below it is called with whatever is assigned to clipPath, which in my case is a fabric.js klass, and this breaks the following call to this._toObjects>this._toObject when trying to locate a methodName on the klass, but the methodName is also a klass assigned to clipPath.

    /**
     * @private
     */
    _toObjectMethod: function (methodName, propertiesToInclude) {

      var clipPath = this.clipPath, data = {
        version: fabric.version,
        objects: this._toObjects(methodName, propertiesToInclude),
      };
      if (clipPath) {
        /** the line below passes clipPath into should be methodName **/
        data.clipPath = this._toObjectMethod(clipPath, methodName, propertiesToInclude);
      }
      extend(data, this.__serializeBgOverlay(methodName, propertiesToInclude));

      fabric.util.populateWithProperties(this, data, propertiesToInclude);

      return data;
    },

    /**
     * @private
     */
    _toObjects: function(methodName, propertiesToInclude) {
      return this._objects.filter(function(object) {
        return !object.excludeFromExport;
      }).map(function(instance) {
        return this._toObject(instance, methodName, propertiesToInclude);
      }, this);
    },

    /**
     * @private
     */
    _toObject: function(instance, methodName, propertiesToInclude) {
      var originalValue;

      if (!this.includeDefaultValues) {
        originalValue = instance.includeDefaultValues;
        instance.includeDefaultValues = false;
      }
     
     /** it breaks here as methodName is in fact the clipPath klass **/
      var object = instance[methodName](propertiesToInclude);
      if (!this.includeDefaultValues) {
        instance.includeDefaultValues = originalValue;
      }
      return object;
    },

For my use I've done following, which seems to work 🤞

fabric.Canvas.prototype._toObjectMethod = function (methodName, propertiesToInclude) {
  var clipPath = this.clipPath, data = {
    version: fabric.version,
    objects: this._toObjects(methodName, propertiesToInclude),
  };
  if (clipPath) {
    data.clipPath = clipPath;
  }
  fabric.util.object.extend(data, this.__serializeBgOverlay(methodName, propertiesToInclude));

  fabric.util.populateWithProperties(this, data, propertiesToInclude);

  return data;
};

I'm probably misunderstanding the code here, so any clarification would be useful.

Thanks

[asturur]

well i do not know why this should be so broken and things works.
That should be clipPath.toObjectMethod() maybe a typo?
That should be either _toObject ... or i do not know.
I would expect UTs to catch it.

[asturur]

Definetely a bug....
that should be _toObject.

Do you mind opening a PR with a test that fails and then add the fix and make test passing?

[asturur]

You should still try to reproduce the bug. I bet that if you add a canvas with a clipPath and then you do toObject, it will fail. won't it?

[asturur]

http://jsfiddle.net/52sp0hev/
^^^ this will make error indeed.

[anthify]

@asturur, sure will open a PR

[anthify]

@asturur should I branch from master? A lot of unit tests are already failing. Is there a recommended node version I should be using with this repo?

[asturur]

any node between 4 and 6.
Tests do not fail if you install canvas properly and build before testing.

npm install
npm run build
npm run test

[anthify]

Cool, will sort it out tomorrow.

[anthify]

@asturur PR Raised #5556

[asturur]

this is fixed now