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

feat: Make Controller the middleware API #2290

Merged
merged 5 commits into from
Nov 22, 2022
Merged

feat: Make Controller the middleware API #2290

merged 5 commits into from
Nov 22, 2022

Conversation

ntucker
Copy link
Collaborator

@ntucker ntucker commented Nov 19, 2022

Motivation

Simplify interface, make easier to write managers

Solution

Since Controller is now a superset of the normal middlware API, we can just send its entirety

import type { Controller, Manager, Middleware } from '@rest-hooks/core';
import type { EndpointInterface } from '@rest-hooks/endpoint';

export default class WebsocketManager implements Manager {
  protected declare middleware: Middleware;
  protected declare websocket: WebSocket;
  protected declare endpoints: Record<string, EndpointInterface>;
  protected declare controller: Controller;

  constructor(url: string, endpoints: Record<string, EndpointInterface>) {
    this.websocket = new WebSocket(url);
    this.endpoints = endpoints;

    this.middleware = controller => {
      this.controller = controller;
      this.websocket.onmessage = this.handleMessage;
      return next => async action => next(action);
    };
  }

  cleanup() {
    this.websocket.close();
  }

  getMiddleware<T extends WebsocketManager>(this: T) {
    return this.middleware;
  }

  handleMessage = (event: MessageEvent<any>) => {
    const { args, data } = JSON.parse(event.data);
    this.controller.receive(this.endpoints[event.type], ...args, data);
  };
}

Note, since we previously added controller as one of the members, we have to adapt the controller in applyManagers like so:

const API = Object.create(controller, {
  controller: { value: controller },
});
middleware(API)

Open questions

This technically breaks someone trying to call middlewares themselves outside of rest hooks, but I cannot imagine a use case for that.

@ntucker ntucker force-pushed the managers branch 2 times, most recently from f7327ed to a875319 Compare November 20, 2022 00:10
@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2022

Codecov Report

Merging #2290 (e665255) into master (b1be0e1) will decrease coverage by 0.09%.
The diff coverage is 96.00%.

@@            Coverage Diff             @@
##           master    #2290      +/-   ##
==========================================
- Coverage   98.31%   98.21%   -0.10%     
==========================================
  Files         125      125              
  Lines        1955     1963       +8     
  Branches      293      295       +2     
==========================================
+ Hits         1922     1928       +6     
- Misses         16       18       +2     
  Partials       17       17              
Impacted Files Coverage Δ
packages/core/src/manager/PollingSubscription.ts 100.00% <ø> (ø)
...ore/src/state/legacy-actions/createReceiveError.ts 85.71% <50.00%> (+2.38%) ⬆️
packages/core/src/controller/Controller.ts 95.28% <100.00%> (+0.09%) ⬆️
packages/core/src/manager/NetworkManager.ts 94.94% <100.00%> (-1.93%) ⬇️
packages/core/src/manager/SubscriptionManager.ts 100.00% <100.00%> (ø)
packages/core/src/manager/applyManager.ts 100.00% <100.00%> (ø)
packages/react/src/components/CacheStore.tsx 100.00% <100.00%> (ø)
packages/react/src/hooks/index.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ntucker ntucker changed the title Managers feat: Supply full controller to managers Nov 20, 2022
@ntucker ntucker force-pushed the managers branch 2 times, most recently from 370f7fc to d2a188b Compare November 20, 2022 02:56
@ntucker ntucker changed the title feat: Supply full controller to managers feat: Make Controller the middleware API Nov 20, 2022
@ntucker ntucker force-pushed the managers branch 2 times, most recently from e665255 to 0bd727c Compare November 21, 2022 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants