Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

turned Query into a class #129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jimmywarting
Copy link
Contributor

got surprised that it still runs fine after mostly auto converting it using vscode
had to make few minor adjustment, added a proxy to make it still work without using new keyword
also got rid of inherit


// TODO
// test utils
module.exports = exports = new Proxy(Query, withoutNew);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would strongly prefer to not export a proxy here, that seems like an unnecessary perf impact.

A couple of alternatives that we can consider:

  1. Implement a class QueryBase { ... }, and then create a function that inherits from that class:
class Base {
  constructor(name) {
    this.name = name;
  }

  sayHi() {
    console.log(`Hello, ${this.name}`);
  }
}

function Child(name) {
  if (!(this instanceof Child)) {
    return new Child(name);
  }
  this.name = name;
}

Child.prototype = Object.create(Base.prototype);
Child.prototype.constructor = Child;

const c = Child('World');
c.sayHi();
  1. Put this in a breaking change, and perhaps add a separate mquery.new() function that allows creating a query instance without using the new operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not have anything against a major change.
mquery.new() seems a bit unnecessary...? and abnormal? in that case we can just say: hey use new Query() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would strongly prefer to not export a proxy here, that seems like an unnecessary perf impact.

Is it really impacting the perf that much?

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

Successfully merging this pull request may close these issues.

2 participants