Enforce privacy for stream internals?

[wabain]

At the moment the polyfill follows the reference implementation in keeping private instance data in fields with leading underscores:

export class ReadableStream<R = any> {
  /** @internal */
  _state!: ReadableStreamState;
  /** @internal */
  _reader: ReadableStreamReader<R> | undefined;
  /** @internal */
  _storedError: any;
  /** @internal */
  _disturbed!: boolean;
  /** @internal */
  _readableStreamController!: ReadableStreamDefaultController<R> | ReadableByteStreamController;

  // ...
}

For some use cases it would be nice to have the internals hidden in a more enforced way. An obvious approach would be to use ES private fields, but polyfilling them accurately requires WeakMaps, so it would be necessary to have some kind of additional processing using something like the babel loose transform mode to support pure ES5 targets (without actually enforcing privacy for them, which seems reasonable). I don't think TypeScript offers this itself at the moment.

Would you be interested in accepting a patch along those lines? Obviously adopting this for the whole polyfill would be a bit of an invasive change.

[MattiasBuelens]

I understand where you're coming from, I would also prefer the polyfill to keep its internals a bit more private. However, I am worried about the downleveling to ES5. Even with Babel's loose mode, each property access or method call becomes an ugly call through a Babel helper function... 😞

An alternative solution would be to use a "semi-private" Symbol to hold the actual implementation, and have the public properties and methods go through this symbol. Something like:

class ReadableStreamImpl {
  _state: ReadableStreamState;
  // ...
}

const impl = Symbol('impl');
class ReadableStream {
  readonly [impl]: ReadableStreamImpl;
  constructor() {
    this[impl] = new ReadableStreamImpl();
  }
  
  getReader() {
    return this[impl].getReader();
  }
}

You could still find the private implementation through Object.getOwnPropertySymbols(), so it wouldn't be truly private. For ES5, we could turn Symbol('impl') into impl or _impl or whatever, so it'd still be a valid property key.

This is kind of how the reference implementation works today, but they use webidl2js to generate the boilerplate code in ReadableStream. I don't know if I'd want that for the polyfill though... 😬

[wabain]

That looks like a reasonable approach! I'll take a look at what the webidl2js transform does.

[jimmywarting]

Just a heads up, looks like it would produce similar problem like the one i just found out about in #75 (about using two different versions)
Some stuff can be truly private but not everything

[MattiasBuelens]

For #75, I would rather restrict the polyfill to only accept streams from its own version, rather than trying to make multiple versions compatible with each other. For example, readableFromVersion1.pipeTo(writableFromVersion2) should always immediately reject with a TypeError here, indicating that the given writable is not valid.

This should still work even if we decide to use private symbols or private fields. But I'll keep in mind to add a test for that. 😁