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

lib: add support for JSTransferable as a mixin #38383

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 24, 2021

@addaleax ... Very interested in what you think on this... the use case is that I want to define internal classes that are both transferable and extend NodeEventTarget but without forcing all NodeEventTarget instances to be cloneable or incur the cost of extending JSTransferable. Because we can't use multiple inheritance this uses JSTransferable as a kind of mixin.

This is not a public facing API. It is inteded for internal use only for now.


Adds a new makeTransferable() utility that can construct a
JSTransferable object that does not directly extend the
JSTransferable JavaScript class.

Because JavaScript does not support multiple inheritance, it is
not possible (without help) to implement a class that extends
both JSTransferable and, for instance, EventTarget without
incurring a significant additional complexity and performance
cost by making all EventTarget instances extend JSTransferable...

That is, we don't want:

class EventTarget extends JSTransferable { ... }

The makeTransferable() allows us to create objects that are
backed internally by JSTransferable without having to actually
extend it by leveraging the magic of Reflect.construct().

const {
  JSTransferable,
  kClone,
  kDeserialize,
  makeTransferable,
} = require('internal/worker/js_transferable');

class E {
  constructor(b) {
    this.b = b;
  }
}

class F extends E {
  constructor(b) {
    super(b);
    return makeTransferable(this);
  }

  [kClone]() { /** ... **/ }
  [kDeserialize]() { /** ... **/ }
}

const f = new F();

f instanceof F;  // true
f instanceof E;  // true
f instanceof JSTransferable;  // false

const mc = new MessageChannel();
mc.port1.onmessage = ({ data }) => {
  data instanceof F;  // true
  data instanceof E;  // true
  data instanceof JSTransferable;  // false
};
mc.port2.postMessage(f);  // works!

The additional internal/test/transfer.js file is required for the
test because successfully deserializing transferable classes requires
that they be located in lib/internal for now.

Signed-off-by: James M Snell [email protected]

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. worker Issues and PRs related to Worker support. labels Apr 24, 2021
@addaleax
Copy link
Member

@jasnell I think this is fine to do, yes 👍

@devsnek
Copy link
Member

devsnek commented Apr 24, 2021

maybe i'm misunderstanding what kConstructor does but would it be possible to instead of doing the following:

class X {
  constructor() {
    this.a = 1;
  }
  [kConstructor]() { return makeTransferable(X) }
}

do this?

class X {
  constructor() {
    this.a = 1;
    return makeTransferable(this);
  }
}

@jasnell
Copy link
Member Author

jasnell commented Apr 25, 2021

maybe i'm misunderstanding what kConstructor does but would it be possible to instead of doing the following:

Yep, absolutely could. I wasn't yet sure if we'd always want X to always be transferable but I've been going through a range of cases and using the simpler option makes sense :-)

@jasnell jasnell marked this pull request as ready for review April 25, 2021 00:40
@jasnell
Copy link
Member Author

jasnell commented Apr 25, 2021

@devsnek ... updated to use the simpler constructor form! This should be ready for review.

@nodejs-github-bot

This comment has been minimized.

@jasnell jasnell force-pushed the jstransferable_mixin branch 2 times, most recently from 5ca4d52 to c915542 Compare April 25, 2021 15:42
@nodejs-github-bot

This comment has been minimized.

Adds a new `makeTransferable()` utility that can construct a
`JSTransferable` object that does not directly extend the
`JSTransferable` JavaScript class.

Because JavaScript does not support multiple inheritance, it is
not possible (without help) to implement a class that extends
both `JSTransferable` and, for instance, `EventTarget` without
incurring a significant additional complexity and performance
cost by making all `EventTarget` instances extend `JSTransferable`...

That is, we *don't* want:

```js
class EventTarget extends JSTransferable { ... }
```

The `makeTransferable()` allows us to create objects that are
backed internally by `JSTransferable` without having to actually
extend it by leveraging the magic of `Reflect.construct()`.

```js
const {
  JSTransferable,
  kClone,
  kDeserialize,
  kConstructor,
  makeTransferable,
} = require('internal/worker/js_transferable');

class E {
  constructor(b) {
    this.b = b;
  }
}

class F extends E {
  [kClone]() { /** ... **/ }
  [kDeserialize]() { /** ... **/ }

  static [kConstructor]() { return makeTransferable(F); }
}

const f = makeTransferable(F, 1);

f instanceof F;  // true
f instanceof E;  // true
f instanceof JSTransferable;  // false

const mc = new MessageChannel();
mc.port1.onmessage = ({ data }) => {
  data instanceof F;  // true
  data instanceof E;  // true
  data instanceof JSTransferable;  // false
};
mc.port2.postMessage(f);  // works!
```

The additional `internal/test/transfer.js` file is required for the
test because successfully deserializing transferable classes requires
that they be located in `lib/internal` for now.

Signed-off-by: James M Snell <[email protected]>
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 25, 2021

@jasnell jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Apr 26, 2021
jasnell added a commit that referenced this pull request Apr 26, 2021
Adds a new `makeTransferable()` utility that can construct a
`JSTransferable` object that does not directly extend the
`JSTransferable` JavaScript class.

Because JavaScript does not support multiple inheritance, it is
not possible (without help) to implement a class that extends
both `JSTransferable` and, for instance, `EventTarget` without
incurring a significant additional complexity and performance
cost by making all `EventTarget` instances extend `JSTransferable`...

That is, we *don't* want:

```js
class EventTarget extends JSTransferable { ... }
```

The `makeTransferable()` allows us to create objects that are
backed internally by `JSTransferable` without having to actually
extend it by leveraging the magic of `Reflect.construct()`.

```js
const {
  JSTransferable,
  kClone,
  kDeserialize,
  kConstructor,
  makeTransferable,
} = require('internal/worker/js_transferable');

class E {
  constructor(b) {
    this.b = b;
  }
}

class F extends E {
  [kClone]() { /** ... **/ }
  [kDeserialize]() { /** ... **/ }

  static [kConstructor]() { return makeTransferable(F); }
}

const f = makeTransferable(F, 1);

f instanceof F;  // true
f instanceof E;  // true
f instanceof JSTransferable;  // false

const mc = new MessageChannel();
mc.port1.onmessage = ({ data }) => {
  data instanceof F;  // true
  data instanceof E;  // true
  data instanceof JSTransferable;  // false
};
mc.port2.postMessage(f);  // works!
```

The additional `internal/test/transfer.js` file is required for the
test because successfully deserializing transferable classes requires
that they be located in `lib/internal` for now.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #38383
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Apr 26, 2021

Landed in bbe24e2

@jasnell jasnell closed this Apr 26, 2021
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 26, 2021
targos pushed a commit that referenced this pull request Apr 29, 2021
Adds a new `makeTransferable()` utility that can construct a
`JSTransferable` object that does not directly extend the
`JSTransferable` JavaScript class.

Because JavaScript does not support multiple inheritance, it is
not possible (without help) to implement a class that extends
both `JSTransferable` and, for instance, `EventTarget` without
incurring a significant additional complexity and performance
cost by making all `EventTarget` instances extend `JSTransferable`...

That is, we *don't* want:

```js
class EventTarget extends JSTransferable { ... }
```

The `makeTransferable()` allows us to create objects that are
backed internally by `JSTransferable` without having to actually
extend it by leveraging the magic of `Reflect.construct()`.

```js
const {
  JSTransferable,
  kClone,
  kDeserialize,
  kConstructor,
  makeTransferable,
} = require('internal/worker/js_transferable');

class E {
  constructor(b) {
    this.b = b;
  }
}

class F extends E {
  [kClone]() { /** ... **/ }
  [kDeserialize]() { /** ... **/ }

  static [kConstructor]() { return makeTransferable(F); }
}

const f = makeTransferable(F, 1);

f instanceof F;  // true
f instanceof E;  // true
f instanceof JSTransferable;  // false

const mc = new MessageChannel();
mc.port1.onmessage = ({ data }) => {
  data instanceof F;  // true
  data instanceof E;  // true
  data instanceof JSTransferable;  // false
};
mc.port2.postMessage(f);  // works!
```

The additional `internal/test/transfer.js` file is required for the
test because successfully deserializing transferable classes requires
that they be located in `lib/internal` for now.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #38383
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
@targos targos removed the semver-minor PRs that contain new features and should be released in the next minor version. label May 3, 2021
@targos
Copy link
Member

targos commented May 3, 2021

I'm removing the semver-minor label as:

This is not a public facing API. It is inteded for internal use only for now.

@targos targos mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. test Issues and PRs related to the tests. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants