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

Structures.extend doesn't search the prototype chain #3215

Closed
Dragoteryx opened this issue Apr 17, 2019 · 3 comments · Fixed by #3217
Closed

Structures.extend doesn't search the prototype chain #3215

Dragoteryx opened this issue Apr 17, 2019 · 3 comments · Fixed by #3217

Comments

@Dragoteryx
Copy link
Contributor

Dragoteryx commented Apr 17, 2019

Is your feature request related to a problem? Please describe.
I started to work on my own Discord.js framework, and I created a mix-in called CommandManager.

// my CommandManager mixin function, it takes a superclass as a parameter and returns a new class that inherits properties from that superclass
CommandManager = superclass => class extends superclass {
  constructor(...args) {
    super(...args);
  }
  createCommand(init, run, options) {
    let command = new Command(this, init, run, options);
    prv(this).commands.set(command.name, command);
    return command;
  }
}

So this is what my Guild extender looks like:

module.exports = Structures.extend("Guild", Guild => {
  class GungnirGuild extends CommandManager(Guild) {
    constructor(...args) {
      super(...args);
    }
  }
  return GungnirGuild;
});

Simple enough, this creates a new class called GungnirGuild, which inherits properties from CommandManager, which inherits properties from Guild. Except the check inside the extend function throws an Error, because it checks for the prototype of the returned class and doesn't check its prototype chain.

Describe the ideal solution
Use instanceof instead of directly comparing the prototypes.
Replace this:

const prototype = Object.getPrototypeOf(extended);
    if (prototype !== structures[structure]) {
      const received = `${extended.name || 'unnamed'}${prototype.name ? ` extends ${prototype.name}` : ''}`;
      throw new Error(
        'The class/prototype returned from the extender function must extend the existing structure class/prototype' +
        ` (received function ${received}; expected extension of ${structures[structure].name}).`
      );
    }

with this:

if (!(extended.prototype instanceof structures[structure])) {
      const prototype = Object.getPrototypeOf(extended);
      const received = `${extended.name || 'unnamed'}${prototype.name ? ` extends ${prototype.name}` : ''}`;
      throw new Error(
        'The class/prototype returned from the extender function must extend the existing structure class/prototype' +
        ` (received function ${received}; expected extension of ${structures[structure].name}).`
      );
    }

This will check if the extended class is in the prototype chain, and will work even if the returned class doesn't directly inherit from the extended class.

Describe alternatives you've considered
Defining my command related functions/properties on every class I want them to be available, but that would take a while and is an anti-pattern.

@Dragoteryx
Copy link
Contributor Author

Dragoteryx commented Apr 17, 2019

Another example that doesn't use mixins:

module.exports = Structures.extend("Guild", Guild => {
  class CoolGuild extends Guild {}
  class CoolerGuild extends CoolGuild {}
  return CoolerGuild;
});

Even though CoolerGuild extends Guild (indirectly), Structures.extend will return an error. Using instanceof fixes this.

@Dragoteryx Dragoteryx changed the title Structures.extend is not compatible with mixins Structures.extend doesn't search the prototype chain Apr 17, 2019
@bdistin
Copy link
Contributor

bdistin commented Apr 17, 2019

I don't see a problem with your proposed solution, however, I am confused on why this is an issue instead of a pr since you are proposing a solution?

@Dragoteryx
Copy link
Contributor Author

Dragoteryx commented Apr 17, 2019

Oh yeah, you're right. Just realised that lol. Going to make a push request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants