Feature Request: Synchronous Worker Client methods

wSedlacek commented 4 years ago

It would be awesome if .observe() and .connect() weren't async. Additionally if .connect() returned the worker client again then this pattern could be used.

@Component({
  selector: 'app-root',
  templateUrl: './app.component.html',
  styleUrls: ['./app.component.css'],
})
export class AppComponent {
  constructor(private workerManager: WorkerManager) {}
  private readonly worker = this.workerManager.createClient(PrimesWorker).connect();
  public readonly outputs$ = this.worker.observe((w) => w.event);
}

Looking at .connect() it looks like it returns a promise because .sendRequest() does https://github.com/gleosmith/angular-web-worker/blob/e599c29e095224c8fbb4e5fd53793b826a998035/angular/src/lib/worker-client.ts#L61-L93

Same with .observe() https://github.com/gleosmith/angular-web-worker/blob/e599c29e095224c8fbb4e5fd53793b826a998035/angular/src/lib/worker-client.ts#L310-L322

And .sendRequest() creates a promise, but I am not sure it needs to. I am not noticing any promises inside it so couldn't it be synchronous? https://github.com/gleosmith/angular-web-worker/blob/e599c29e095224c8fbb4e5fd53793b826a998035/angular/src/lib/worker-client.ts#L348-L434

gleosmith commented 4 years ago

No unfortunately not possible, both observe and connect communicate with the worker script through the onmessage and postMessage mechansim which needs to be async. Additionally, the connect method can trigger the onWorkerInit hook which can have an async implementation and is only resolved once this hook is resolved.

wSedlacek commented 4 years ago

After reviewing the code further I see that sendRequest() is using .subscribe() and a callback which is where it's async nature comes from.

I have spent quite some time working with this code and cleaning it up with Strict mode in TypeScript. I have completed most of that work however it did require a considerable amount of additional checks and optional chaining since the constructor() is not providing values and it isn't until connect() is fired that values are assigned.

If you don't mind, I would like to continue to work with this code and try to simplify it so these checks aren't needed for Strict mode.

I would also like to see if I can't find a way to enable that clean pattern I detailed above. When working under TypeScript strict mode (like I do with nearly all my angular projects) it becomes a pain when value initialization needs to be deferred to the ngOnInit() as then you need to account for the value possibly being undefined as technically between the constructor, and the first ngOnInit() (which occurs after the first ngOnChanges() it is undefined.

wSedlacek commented 4 years ago

I have a working prototype for this feature over here.

https://github.com/wSedlacek/angular-web-worker/blob/cleanup/libs/angular-web-worker/src/client/lib/worker-client/worker-client.ts

I took some liberties with the API in attempting to tidy things up a bit.

@Component({
  selector: 'app-root',
  templateUrl: './app.component.html',
  styleUrls: ['./app.component.css'],
})
export class AppComponent {
  constructor(private workerManager: WorkerManager) {}
  private readonly client = this.workerManager.createClient(AppWorker);
  public readonly output$ = this.client.observe((w) => w.output$);

  public pushValue(): void {
    this.client.next((w) => w.input$, `New value ${Date.now()}`);
  }
}

The major changes are:

.connect() was removed and all of it's functionality was wrapped up into the constructor
.subscribe() was removed since .observe() does more or less the same thing and leaves the subscription up to the client encouraging the use of the async pipe.
.observe() returns an Observable (not wrapped in a promise)
.next() was introduced to pass values into workers via subjects.

If you are interested in these features let me know and we get a PR going on this repo. If you aren't interested in these API changes would you mind if I published a fork of this library with these changes?

gleosmith commented 4 years ago

The removal of the async nature .connect() removes the ability to know when the afterWorkerInit() method has been completed (which can be async). An example would be loading data from an external service each time the worker is created, after which you can only call a method or implement some other behavior. There is no specific reason why it can't all be included in createWorker as you have done but it must be async.

I'm not sure on the idea removing .subscribe() but not completely closed to it. A subscription also exists in the worker so only exposing the observable through async pipe does not mean you don't have to worry about unsubscribing as you still need call client.unsubscribe(obs). It can also increase code overhead if need to subscribe in the ts as you will have to unsubscribe from the client by providing the observable and also unsubscribe from the independent subscription. So I guess I am questioning the need to remove something that doesn't really result in a benefit but requires more code in a particular circumstance?

I like the ideas of .observe() not wrapped in a promise and the new feature .next()

I am not keen on all the other changes you have called cleanup. The entirety of the repo structure, code and tools have been changed even to the extent of changing from jasmine to jest. Are there any non-subjective reasons for these changes? There is no reason to essentially replace the repo based on preferences as it creates additional overheads unnecessarily. Happy to consider any changes for issues, new feature, performance enhancements.

wSedlacek commented 4 years ago

Thank you for reviewing this!

I am not keen on all the other changes you have called cleanup. The entirety of the repo structure, code and tools have been changed even to the extent of changing from jasmine to jest. Are there any non-subjective reasons for these changes? There is no reason to essentially replace the repo based on preferences as it creates additional overheads unnecessarily. Happy to consider any changes for issues, new feature, performance enhancements.

Most of the repo and project level changes I made in this example simply to wrap my head around the project with code I was more familiar with. 😅 In all honestly it was quite difficult to understand what was going on at first.

Of course the entire repo structure wouldn't change for a feature PR. I would simply make a few small PR with only the required changes for each feature based on the work I have done here.

Many of the test previously for worker-client were testing internal private methods. I believe testing implantation details is a bit of an Anti Pattern. Source: https://medium.com/@dev.cprice/stop-testing-implementation-details-77a3528336af I stand by rewriting the test in the way I did, more test I believe need rewrites, however using Jest over Jasmine wasn't the important change as these test could be written in either. In all honestly I just used jest because it allowed me to work with a quicker feedback loop.

I do stand by some of my renames, such as @AngularWebWorker() into @WebWorker() and angular-web-worker/angular into angular-web-worker/client These are subjective, but I believe these names are less repetitive and more closely align with the naming structure in other Angular libraries.

The removal of the async nature .connect() removes the ability to know when the afterWorkerInit() method has been completed (which can be async). An example would be loading data from an external service each time the worker is created, after which you can only call a method or implement some other behavior. There is no specific reason why it can't all be included in createWorker as you have done but it must be async.

You make a good point about needing to know when afterWorkerInit is complete. I will rework that.

I'm not sure on the idea removing .subscribe() but not completely closed to it. A subscription also exists in the worker so only exposing the observable through async pipe does not mean you don't have to worry about unsubscribing as you still need call client.unsubscribe(obs). It can also increase code overhead if need to subscribe in the ts as you will have to unsubscribe from the client by providing the observable and also unsubscribe from the independent subscription. So I guess I am questioning the need to remove something that doesn't really result in a benefit but requires more code in a particular circumstance?

As far as .subscribe(), I believe .destroy() removes all these subscriptions and users should be encouraged to always use this as it also calls onWorkerDestory(), Maybe .unsubscribe() should be removed from the public API to further encourage this.

Update: We could also extend the Observable class and add functionality on the unsubscribe() to also send an event to the Worker. This would allow the async pipe to call .unsubscribe() and actually fire the event all the way through.

wSedlacek commented 4 years ago

You make a good point about needing to know when afterWorkerInit is complete. I will rework that.

Actually after reviewing the changes I have made it is very possible to know when the connection is completed with the new connectionCompleted getter.

An example would be loading data from an external service each time the worker is created, after which you can only call a method or implement some other behavior.

This would look something like this.

public ngOnInit(): void {
  // Promise based for loading after the initial connection. 
  this.client.connectionCompleted.then(() => this.dataService.load());

  // Observable based for loading whenever a new connection is made
  this.client.isConnected$
    .pipe(filter((connected) => connected))
    .subscribe(() => this.dataService.load());
}

Would this be sufficient?

gleosmith commented 4 years ago

An example would be loading data from an external service each time the worker is created, after which you can only call a method or implement some other behavior.

Sorry I want very descriptive in this use case, was meant to say that the loading data would happen in the hook . So you create the worker ->listen to connectionCompleted$ or something similar while the worker loads some external resources in the init hook -> once the worker got it data you can make calls to its methods e.t.c.

But yeah I like your idea, maybe just make connectionCompleted$ an observable so its similar to isConnected$

Where destroy won't be applicable is if you have a WorkerClient managed my a service and various components subscribe (or use async pipe) for observable from the worker. The worker may live as long as the app and in that case destroy may never be called. Although, the resources consumed by the subscriptions in the worker are very minimal as it basically just posts a message back to the client without any heavy computational logic, it still best to ensure that they are cleaned up.

wSedlacek commented 4 years ago

Sorry I want very descriptive in this use case, was meant to say that the loading data would happen in the hook . So you create the worker ->listen to connectionCompleted$ or something similar while the worker loads some external resources in the init hook -> once the worker got it data you can make calls to its methods e.t.c.

In sendRequest() before sending the request connectionCompleted$ is awaited

      if (!opts.isConnectionRequest && !(await this.connectionCompleted$.toPromise())) {
        reject(
          'WorkerClient: Could not connect to the worker... Has it already been destroyed it?'
        );

        return;
      }

This would prevent any request from being made before the workerInit is completed thus I believe the scenario you are describing wouldn't be possible as if you were to make a request while the Init hook was still being settled it would just wait for the connectionCompleted then make the request whenever that was finished.

Where destroy won't be applicable is if you have a WorkerClient managed my a service and various components subscribe (or use async pipe) for observable from the worker. The worker may live as long as the app and in that case destroy may never be called. Although, the resources consumed by the subscriptions in the worker are very minimal as it basically just posts a message back to the client without any heavy computational logic, it still best to ensure that they are cleaned up.

You do make a very good point here. I am going to attempt the extending the Observable class with hooks and see what that looks like. I do believe we can find a way to tap into when .unsubscribed() is called on an observable and simply fire request when that happens to make the process seamless for consumers of this library.

wSedlacek commented 4 years ago

I am going to attempt the extending the Observable class with hooks and see what that looks like.

Maybe something like this with proxies? createHookedSubject WorkerClient

The resulting behavior is whenever .unsubscribe() is called from the resulting subscription from the observable created with .observe() it would run the request to unsubscribe the observable in the worker if all subscriptions are closed thus completely removing the need for the consumers of the library to do anything special.

gleosmith / angular-web-worker

Feature Request: Synchronous Worker Client methods #7