Extending in ES6?

[claym]

I'm using 0.8.1

Trying to figure out how to write a Bookshelf model in ES6.

if I do it like this:

export default class Account extends Bookshelf.Model {
    constructor() {
        super({
            tableName: "account",
            idAttribute: "id"
        });
    }
}

the tableName does get set and I get lots of undefined errors.

If I try like

export default class Account extends Bookshelf.Model {
    constructor() {
        super();
        this.tableName = 'account';
        this.idAttribute = 'id';
    }
}

It actually gets the table name, but it doesn't seem to be registering the id or any other column names, any query is just "select account.* from account".

Any suggestions?

[bendrucker]

Bookshelf expects tableName to be on the prototype and it's possible idAttribute is the same. There is no way to add prototype properties with the class syntax, including under the current ES7 class properties. Bookshelf models don't really play nicely with class. You have to manually add everything to the prototype after the class is defined.

[claym]

So, best just not to use classes? That's disappointing. 

-Clay

On Wed, May 27, 2015 at 8:36 AM, Ben Drucker notifications@github.com
wrote:

Bookshelf expects tableName to be on the prototype and it's possible idAttribute is the same. There is no way to add prototype properties with the class syntax, including under the current ES7 class properties. Bookshelf models don't really play nicely with class. You have to manually add everything to the prototype after the class is defined.

Reply to this email directly or view it on GitHub:
#756 (comment)

[luggage66]

I use ES6 with something like:

export class ProductionSheet extends bookshelf.Model
{
    get tableName() { return 'ProductionSheet'; }

    get defaults() {
        return {
            property1: 'blah',
            property1: null
        }
    }

    // START Relations
    timesheet() { return this.belongsTo('Timesheet', 'timesheet'); }
    taskType() { return this.belongsTo('TaskType', 'taskType'); }
    worksheets() { return this.hasMany('AuditWorksheet', 'productionSheet'); }
    // END Relations
}

[olalonde]

Bump. It would be nice if there was something in the docs on using es6 class syntax.

[luggage66]

There are still problems with ES6, especially collections. There are some static functions used that don't get 'inherited'. Models work, though, for the most part.

[olalonde]

Strange, which static functions don't get inherited? Just glanced at the code and couldn't see anything funky. The extend() method just seems to create a new constructor and copy passed properties to its prototype or directly on the constructor (static functions). Isn't what class Child extends Parent does? Also, why do some static function get inherited while others don't?

[olalonde]

Yeah, just converted some models using the method you used above and getting weird errors.

  sql: 'insert into "outputs" ("0", "position", "tx_hash") values ($1, $2, $3)',
  returning: undefined }
{ __cid: '__cid16',
  method: 'insert',
  options: undefined,
  bindings:
   [ { amount: 498369000,
       script: [Object],
       script_type: 'pubkeyhash',
       address: 'msLoJikUfxbc2U5UhRSjc2svusBSqMdqxZ' },
     2,
     '431d6c31dcb8414432d61f9ff5eb67fd6cb2c5f91d9748718c411b86a58e3874' ],
  sql: 'insert into "outputs" ("0", "position", "tx_hash") values ($1, $2, $3)',
  returning: undefined }

when trying to output.save()

My output class has this method:

    get idAttribute() { return null; }

[olalonde]

Oops, the problem was that I did:

    constructor(...args) {
      super(args);
   }

instead of

    constructor(...args) {
      super(...args);
    }

[olalonde]

Ok all the tests pass. Will give an update if I encounter problems.

[luggage66]

No, the (ES6) extends only does the instance properties (the prototype). None of the statics. Coffeescripts's extends does copy both the properties on the prototype and the constructor just like the supplied .extend().

I don't remember what was broken on the collections but for models, the fetch option { require: true } fails due to the errors it uses being static properties on the model: https://github.com/tgriesser/bookshelf/blob/master/src/model.js#L1219

[rhys-vdw]

...fails due to the errors it uses being static properties on the model...

@luggage66 Actually ES6 extends does do static inheritance - the inheriting class will take the static properties of the parent.

class Test {
  static test() { return "hello";}
}

class Child extends Test {}

console.log(Child.test());
// "hello"

The problem is that Bookshelf's extend method has an additional functionality that decorates the class with specific instantiations of these errors. Because ES6 offers no static constructor, I don't know that we can recreate this exact API using the new class syntax. However this is certainly something I'm keeping in mind moving forward.

The rationale behind having this sublcassed errors is for catching by type in Bluebird. eg.

User.forge({id: 1}).fetch({require: true}).then(function (user) {
  return user.account().fetch({require: true});
}).catch(User.NotFoundError, function (error) {
  console.log("User not found!");
}).catch(Account.NotFoundError, function (error) {
  console.log("Account not found!");
}).catch(function (error) { console.log("Unknown error occurred: " + error); });

I have two possible solutions for this. I prefer the second.

1. Lazily create error classes

Instead of having Model.NotFoundError, use Model.error('NotFound') to return the error instance. Since there is no static constructor, we'll have to define the error the first time this is called. So something like this:

class Model {
  // ...
  static error(errorName) {
    errorClass = `${errorName}Error`;
    if (this[errorClass] == null) {
      this[errorClass] = class extends super.error(errorName);
    }
  }
}

So that allows the above use of catch by type, like this:

User.forge({id: 1}).fetch({require: true})
.then((user) => user.account().fetch({require: true}))
.catch(User.error('NotFound'), (error) =>
  console.log("User not found!")
);

2. Give model type to errors

Provide a model type like so:

class NotFoundError extends Error {
  constructor: (message, Model) {
    this.Model = Model;
    super(message);
  }

  static forModel(Model) {
    return (error) => error instanceof this && error.model = Model;
  }
}

That class just lives at bookshelf.NotFoundError and is always fired with the model constructor as an argument. Then catch like so, taking advantage of Bluebird's catch allowing a callback.

import {NotFoundError} from 'bookshelf';

User.forge({id: 1}).fetch({require: true})
.then((user) => user.account().fetch({require: true}))
.catch(NotFoundError.forModel(User), (error) =>
  console.log("User not found!");
);

This is not a high priority change yet. There are other features that will be affected by moving to ES6 classes. However, I'd like to get everything right, so feedback is encouraged. 👍

[luggage66]

UPDATE: The below code is not actually doing what I think. Some coffeescript classes in the chain screwed it up, not ES6 classes which seem to get .forge(), .collection(), etc.

Here is some hackery I am testing out that just copies bookshelf 0.8.2 static properties to the inherited class: https://gist.github.com/luggage66/8920135a556fc33b06ac (it's an es6 decorator, but you can just call it as a function).

It at least copies collection, forge, fetchAll, etc.

[rhys-vdw]

@luggage66 extends already does static inheritance. If you want to get this happening, please consider providing a PR implementing my second proposal from the post above.

AFAIK Decorators are ES7 and the standard has not yet been finalized, and this can be achieved without them anyway.

Just a note on back compatibility here. Errors can be removed from the Model classes entirely, alleviating this problem. However, they were placed on the classes so that they could be used in combination with Bluebird's Promise#catch, which also accepts a predicate as an argument. so we can do something like this:

// import new errors module
import { NotFoundError } from './errors';

// model definition
class Model {
  static NotFoundError(error) {
    deprecate('NotFoundError');
    // Use the predicate on our error class.
    return NotFoundError.forModel(this)(error);
  }

  fetch() {
    // ...
    throw new NotFoundError(this.constructor);
    // ...
  }
}

Are you up for helping out with this change?

edit: Fixed the deprecated NotFoundError example.

[rhys-vdw]

It at least copies collection, forge, fetchAll, etc.

ES6 extend does this already...

[luggage66]

Yea, I had some coffeescript classes in the mix (don't ask) that cause things to break from 0.8.1 -> 0.8.2 and I misinterpreted the evidence.

I finished converting them all to es6 and now I can use 0.8.2 (except for the known Error problem, of course).

Yea, I'll take a look at the fixing the Error types. I think, with static getters, I can generate (and cache) error types on demand so that we can:

.catch(User.NotFoundError, (error) => ....)

or, if you want to catch all NotFoundErrors for any model:

.catch(bookshelf.NotFoundError, error => ...)

without the special extended() function that creates those types now when model.extent({ instance, static}) is called. That would prevent browser use in IE < 9, if that matters to you.

[tkrotoff]

I've switched all my code from var Account = bookshelf.Model.extend({ ... }) to TypeScript classes (class Account extends bookshelf.Model<Account> { ... }) and all my unit tests pass as before. They heavily test NotFoundError, NoRowsUpdatedError and NoRowsDeletedError; example:

const id = req.body.id;
new Account({id})
  .fetch({require: true})
  .then(account =>
    res.status(HttpStatus.OK_200).send(account.toJSON())
  )
  .catch(Account.NotFoundError, () =>
    next(createError(HttpStatus.NotFound_404, `'${id}' not found`))
  )
  .catch(error => next(error));

it('should not get an unknown account', done => {
  const id = -1;

  request(app).get(`/Accounts/${id}`)
    .expect(HttpStatus.NotFound_404)
    .then(res => {
      expect(res.body).toEqualErrorObject({
        status: HttpStatus.NotFound_404,
        stack: `NotFoundError: '${id}' not found`,
        message: `'${id}' not found`,
        name: 'NotFoundError'
      });
      done();
    });
});

I was surprised and did more investigations and my conclusion is that everything works as expected. Edit: it does not work.

@rhys-vdw

1. Are you sure that "Bookshelf's extend method [...] decorates the class with specific instantiations of these errors" does not work with ES6 classes? Is there a unit test somewhere to confirm?
2. If it does not work as you say, why does it with TypeScript (ES5 output) and not with Babel?

(My previous obsolete and deleted post was: "Meanwhile how to solve the NotFoundError and friends problem while inheriting from Bookshelf.Model?")

[rhys-vdw]

I've never used TypeScript, but if you use ES extend, it would copy the static error classes from Model onto your new subclass. Bookshelf's Model.extend has an additional functionality that does extra work by the magical extended function. Errors get subclassed via Model.extended.

So presumably, using ES inheritance you get this:

class Account extends bookshelf.Model {  /* ... */ }

class User extends bookshelf.Model {  /* ... */ }

assert.equal(Account.NotFoundError, User.NotFoundError);

// The purpose of subclassing being for Bluebird's `catch` by type.
Promise.all(
  Account.where({ id: 1 }).fetch({ require: true }),
  User.where({ id: 1 }).fetch({ require: true })
).catch(Account.NotFoundError, error => /* could be either account or user */ );

The solution is to stop subclassing errors and provide some other method for catching them. We could add a static for() method and an instance of the issuing class to each error.

class NotFoundError extends Error {
  constructor(Model) {
    this.name = 'NotFoundError';
    this.Model = Model;
  }
  static is(maybeError) {
    return maybeError instanceof Error && maybeError.name === this.name;
  }

  static for(Model) {
    return error => this.is(error) && this.Model === Model;
  }
} 

// ...
const { NotFoundError } = bookshelf.Model;

Promise.all(
  Account.where({ id: 1 }).fetch({ require: true }),
  User.where({ id: 1 }).fetch({ require: true }))
.catch(NotFoundError.for(Account), error => console.error('account not found!'))
.catch(NotFoundError.for(User), error => console.error('user not found!'));

[tkrotoff]

.catch(Account.NotFoundError, error => /* could be either account or user */ );

Ok I see; my mistake.

Temporary solution:

import * as createError from 'create-error';

class Account extends bookshelf.Model<Account> {
  get tableName() { return 'Accounts'; }

  // Redefine errors
  static NotFoundError = createError('NotFoundError');
  static NoRowsUpdatedError = createError('NoRowsUpdatedError');
  static NoRowsDeletedError = createError('NoRowsDeletedError');
}

[bogas04]

Sorry if it's off topic, but is there official doc for using ES6 class syntax with Model.extend now that several months have passed ?

[rhys-vdw]

@bogas04 We still need an error implementation and then we can add ES6 examples to the docs. I've added the "PR please" label as I am not actively working on Bookshelf presently. I will review any submissions though.

[kripod]

I started to work on a Knex-based ORM which aims to provide easy model integration with ES6+:
https://github.com/kripod/knexpress

My goal is to achieve feature parity with Bookshelf.js with as few lines of code as possible.

[henryboldi]

@kripod Great work! I'd love to see some of these improvements merged into bookshelf.

[kripod]

@henryboldi Well, Knexpress mounts every Knex method dynamically, while Bookshelf has a predefined set of functions to operate from. Bookshelf was not built to utilize ES6 classes natively, that's why I decided to create a project of my own.

Besides the points above, I would also be very glad to see my class-related ideas being implemented to Bookshelf. (At the moment, I'm working on implementing support for relations in Knexpress.)

[dantman]

Does es6 extends copy static getters as-is?

If so, couldn't the SomeModel.NotFoundError be handled by making NotFoundError a memoized getter that creates the subclass of NotFoundError from this.

[jordaaash]

I found a workaround per jtwebman/bookshelf-scopes#1 (comment)

Calling extended on your model class will cause it to be assigned the desired Error classes and other plugin behavior.

[crunchtime-ali]

The project leadership of Bookshelf recently changed. In an effort to advance the project we close all issues older than one year.

If you think this issue needs to be re-evaluated please post a comment on why this is still important and we will re-open it.

We also started an open discussion about the future of Bookshelf.js here #1600. Feel free to drop by and give us your opinion.
Let's make Bookshelf great again

[dantman]

If you think this issue needs to be re-evaluated please post a comment on why this is still important and we will re-open it.

The importance of this issue should be self-evident. Bookshelf classes still require custom functions to create classes and don't work with ES6 classes and all their new features. Things like the classProperties on extends are obsolete ways of doing things and the extends don't allow you to create getters/setters.

[crunchtime-ali]

Re-opened. Please use thumbs up (:+1:) emoji on the original question so have an easier time prioritising and planning the next steps.

[rapzo]

I agree that all code should be refactored in order to drop the lodash's extend in favour of classes.
I've been using them for years with bookshelf and it plays well (yet to find an issue).

Is anyone heading this refactor?

[ricardograca]

@rapzo I don't think so.

[MartinMuzatko]

I don't see any typescript examples online of bookshelf. How are they supposed to work?