Skip to content

Commit

Permalink
feat: forbid accessing this before super
Browse files Browse the repository at this point in the history
  • Loading branch information
Josh-Cena committed Apr 25, 2022
1 parent 69bf0f1 commit 569cda9
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 26 deletions.
2 changes: 1 addition & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
},
{
"message": "We hate new.",
"selector": "NewExpression:not(:has(Identifier[name=Error]))"
"selector": "NewExpression:not(:has(Identifier[name=Error], Identifier[name=Proxy]))"
},
{
"message": "We hate classes.",
Expand Down
17 changes: 14 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ console.log(dog.location());
Named klasses can have a super klass as well.

```js
const Animal = klass("Animal").extends(Entity)({
const Animal = klass.extends(Entity)({
location() {
return [this.x, this.y];
},
Expand All @@ -141,7 +141,7 @@ const Entity = klass({
},
});

const Animal = klass("Animal").extends(Entity)({
const Animal = klass.extends(Entity)({
greet() {
super.greet();
},
Expand All @@ -159,7 +159,7 @@ const Entity = klass({
},
});

const Animal = klass("Animal").extends(Entity)({
const Animal = klass.extends(Entity)({
constructor() {
super.constructor();
this.b = this.a + 1;
Expand All @@ -169,6 +169,17 @@ const Animal = klass("Animal").extends(Entity)({
console.log(Animal()); // Logs { a: 1, b: 2 }
```

As you would expect, you cannot access `this` before calling `super.constructor`.

```js
const Animal = klass.extends(Entity)({
constructor() {
this.b = this.a + 1; // Throws error
super.constructor();
},
});
```

### Klass name

Unfortunately, because `klass` is ultimately a normal ECMAScript function, there's no great way for us to automatically bind a klass' name based on what it's assigned to. If a klass' name is important to you, you can explicitly bind a name.
Expand Down
44 changes: 34 additions & 10 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,44 @@ function klassCreator(body, name, SuperKlass) {
if (SuperKlass && !isKlass(SuperKlass))
throw new Error("You can only extend klasses.");

let superBeenCalled = false;

function createGuardedThisArg(thisArg) {
return new Proxy(
thisArg,
Object.fromEntries(
Object.getOwnPropertyNames(Reflect).map((k) => [
k,
(...opArgs) => {
if (!superBeenCalled) {
throw new Error(
`You must call super.constructor() in derived klass before performing '${k}' on 'this'.`,
);
}
return Reflect[k](...opArgs);
},
]),
),
);
}

const constructor = Object.hasOwn(body, "constructor")
? (() => {
const customCtor = body.constructor;
delete body.constructor;
// eslint-disable-next-line no-restricted-syntax
return new Proxy(customCtor, {
apply(target, thisArg, args) {
defineProperties(thisArg, instanceFields);
Reflect.apply(target, thisArg, args);
Reflect.apply(
target,
SuperKlass ? createGuardedThisArg(thisArg) : thisArg,
args,
);
if (SuperKlass && !superBeenCalled) {
throw new Error(
"You must call super.constructor() in derived klass before returning from derived constructor.",
);
}
return thisArg;
},
});
Expand All @@ -79,25 +108,20 @@ function klassCreator(body, name, SuperKlass) {
if (SuperKlass) {
Object.setPrototypeOf(SomeKlass, SuperKlass);
Object.setPrototypeOf(SomeKlass.prototype, SuperKlass.prototype);
// For accessing `super` in the body. We don't properly supper `super` in
// For accessing `super` in the body. We don't properly support `super` in
// static methods yet, but we should figure out a way to (probably through
// chaining another `.static({})` block that binds another prototype)
Object.setPrototypeOf(
body,
// eslint-disable-next-line no-restricted-syntax
new Proxy(SuperKlass.prototype, {
get(target, property) {
if (property !== "constructor") return Reflect.get(target, property);
// SuperKlass.prototype.constructor is actually SuperKlass itself, but
// we proxy it to the constructor function, so that we can support
// `super()` with `super.constructor()`
// eslint-disable-next-line no-restricted-syntax
// calling `super()` through `super.constructor()`
return new Proxy(Constructors.get(SuperKlass), {
apply(ctor, thisArg, args) {
// TODO we should try our best to disallow accessing this before
// super. We probably can't reach full ES-compliance, but it at
// least avoids footguns
// superBeenCalled = true;
superBeenCalled = true;
Reflect.apply(ctor, thisArg, args);
return thisArg;
},
Expand Down
50 changes: 38 additions & 12 deletions test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -532,23 +532,47 @@ describe("super call", () => {
// No name, so that it serializes to `Object {...}`
const Animal = klass.extends(Entity)({
constructor() {
// TODO should we disallow this before super?
super.constructor();
this.b = 2;
this.same = 3;
super.constructor();
},
});
const Animal2 = klass.extends(Entity)({
const dog = Animal();
expect(dog).toEqual({ a: 1, b: 2, same: 3 });
});

it("needs to happen before accessing this", () => {
const Entity = klass("Entity")({});
const Animal = klass.extends(Entity)({
constructor() {
super.constructor();
this.b = 2;
this.same = 3;
},
});
const dog = Animal();
const dog2 = Animal2();
expect(dog).toEqual({ a: 1, b: 2, same: 5 });
expect(dog2).toEqual({ a: 1, b: 2, same: 3 });
expect(() => Animal()).toThrowErrorMatchingInlineSnapshot(
`"You must call super.constructor() in derived klass before performing 'set' on 'this'."`,
);
const Animal2 = klass.extends(Entity)({
constructor() {
console.log(this.a);
},
});
expect(() => Animal2()).toThrowErrorMatchingInlineSnapshot(
`"You must call super.constructor() in derived klass before performing 'get' on 'this'."`,
);
const Animal3 = klass.extends(Entity)({
constructor() {
console.log(Object.keys(this));
},
});
expect(() => Animal3()).toThrowErrorMatchingInlineSnapshot(
`"You must call super.constructor() in derived klass before performing 'ownKeys' on 'this'."`,
);
const Animal4 = klass.extends(Entity)({
constructor() {},
});
expect(() => Animal4()).toThrowErrorMatchingInlineSnapshot(
`"You must call super.constructor() in derived klass before returning from derived constructor."`,
);
});

describe("execution order is the same as class", () => {
Expand All @@ -568,7 +592,6 @@ describe("super call", () => {
test("class", () => {
let foo = undefined;
const Entity = class {
/** @member {number} a */
constructor() {
foo = this.a;
}
Expand All @@ -588,11 +611,14 @@ describe("super call", () => {
},
});
const B = klass.extends(A)({
a: 1,
method() {
return super.method();
// Testing order: 'this' should be allowed before 'super'
const b = this.a;
return super.method() + b;
},
});
expect(B().method()).toBe(1);
expect(B().method()).toBe(2);
});
});

Expand Down

0 comments on commit 569cda9

Please sign in to comment.