tvkitchen / countertop

The entry point for developers who want to set up a TV Kitchen.
https://tv.kitchen
GNU Lesser General Public License v3.0
6 stars 2 forks source link

Not implemented error for _transform, _write methods in AbstractIngestionEngine #57

Closed chriszs closed 4 years ago

chriszs commented 4 years ago

Bug

Current Behavior

When I try to inherit from the AbstractIngestionEngine and use it to stream from a file, I get:

_stream_transform.js:166
  throw new ERR_METHOD_NOT_IMPLEMENTED('_transform()');
  ^

Error [ERR_METHOD_NOT_IMPLEMENTED]: The _transform() method is not implemented
    at Transform._transform (_stream_transform.js:166:9)
    at Transform._read (_stream_transform.js:191:10)
    at Transform._write (_stream_transform.js:179:12)
    at writeOrBuffer (_stream_writable.js:352:12)
    at Transform.Writable.write (_stream_writable.js:303:10)
    at Socket.ondata (_stream_readable.js:712:22)
    at Socket.emit (events.js:315:20)
    at Socket.EventEmitter.emit (domain.js:485:12)
    at addChunk (_stream_readable.js:302:12)
    at readableAddChunk (_stream_readable.js:278:9) {
  code: 'ERR_METHOD_NOT_IMPLEMENTED'
}

Input

yarn babel-node src/components/ingestion/test.js
// src/components/ingestion/test.js
import 'module-alias/register'
import AbstractIngestionEngine from './AbstractIngestionEngine'

class FileIngestionEngine extends AbstractIngestionEngine {
    constructor(path) {
        super()

        this.path = path
    }

    path = null

    getInputStream = () => fs.createReadStream(this.path)
}

const engine = new FileIngestionEngine(__dirname + '/in.ts')

engine.start()

Expected Behavior

I expected it to not throw that error.

Possible Solutions

There are two issues:

  1. The transform stream expects a transform method, but gets a write method instead.
  2. As properties, payloadIngestionStream and mpegtsProcessingStream are instantiated before the class is and therefore the references to methods with this aren't defined. Could move instantiation to either constructor or start. This may be a good idea with properties generally, since we want new copies on each instantiation.
slifty commented 4 years ago

nuts! I wonder if this can be reproduced with a unit test or if we'll need integration tests to patch this one.

Regarding the second item: when I implemented as a class property I was trying to decide what was cleaner and this answer led me to the class properties approach.

I was under the impression that class properties are just syntactic sugar and are ultimately creating new copies on each instantiation / don't get instantiated before the class. What you're saying makes it seem like that's not the case (happy to be wrong, but want to sanity check)!

chriszs commented 4 years ago

It’s worth checking that part of it. What I know for sure is this wasn’t available.

chriszs commented 4 years ago

This was also a method order issue, same as #60.

slifty commented 4 years ago

Closed by #56