Skip to content
This repository was archived by the owner on Feb 7, 2024. It is now read-only.

[2.x] Add Custom API Route Handlers #662

Closed
wants to merge 3 commits into from

Conversation

simonbuehler
Copy link
Contributor

hi,

this should fix #660

@codecov-io
Copy link

codecov-io commented Jan 15, 2021

Codecov Report

Merging #662 (90103d5) into 2.x (8baefdd) will decrease coverage by 0.09%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x     #662      +/-   ##
============================================
- Coverage     90.71%   90.61%   -0.10%     
  Complexity        2        2              
============================================
  Files            60       60              
  Lines          1690     1694       +4     
============================================
+ Hits           1533     1535       +2     
- Misses          157      159       +2     
Impacted Files Coverage Δ Complexity Δ
src/Server/Router.php 78.37% <50.00%> (-3.44%) 0.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8baefdd...90103d5. Read the comment docs.

@simonbuehler simonbuehler changed the title 2.x [2.x] Add Custom API Route Handlers Jan 15, 2021
@simonbuehler
Copy link
Contributor Author

@rennokki any plans to merge ?

@rennokki
Copy link
Collaborator

It's a bit tricky because you can already call the \BeyondCode\LaravelWebSockets\Facades\WebSocketRouter::get in your AppServiceProvider's boot method.

@simonbuehler
Copy link
Contributor Author

i see - currently #661 is in the way for the WebSocketRouter::get approach therefore i made it work this way - in the end its a matter of taste if doing this via explicit code or config is preferred

@simonbuehler
Copy link
Contributor Author

Actually it throws even anothe error when trying to bind to Class which extends BeyondCode\LaravelWebSockets\API\Controller

   Illuminate\Contracts\Container\BindingResolutionException

  Target [BeyondCode\LaravelWebSockets\Contracts\ChannelManager] is not instantiable while building [App\API\SelectRandomUserFromChannel].

  at vendor/laravel/framework/src/Illuminate/Container/Container.php:1038
    1034▕         } else {
    1035$message = "Target [$concrete] is not instantiable.";
    1036▕         }
    1037▕
  ➜ 1038▕         throw new BindingResolutionException($message);
    1039▕     }
    1040▕
    1041▕     /**
    1042▕      * Throw an exception for an unresolvable primitive.

      +17 vendor frames
  18  app/Providers/AppServiceProvider.php:31
      Illuminate\Support\Facades\Facade::__callStatic("get")

      +7 vendor frames
  26  [internal]:0
      Illuminate\Foundation\Application::Illuminate\Foundation\{closure}(Object(App\Providers\AppServiceProvider))

@rennokki
Copy link
Collaborator

It's weird because the issue #661 that you sent to me has a different error message than the one you specified now.

The error message that you specified now it's because the ChannelManager class is not instantiable. The sockets cannot be accessed outside the server, only within the files that are strictly used by the start command

Show me a sample of your code using WebSocketRouter::get and the controller you're trying to implement. Having a config file that does that is just duplicating the functionality.

@simonbuehler
Copy link
Contributor Author

Therefor i think my pull request is the only way to hook into the correct server context - for example this class

<?php

namespace App\API;

use BeyondCode\LaravelWebSockets\API\Controller;
use Illuminate\Http\Request;
use Illuminate\Http\Response;
use React\Promise\PromiseInterface;
use function React\Promise\all;


class GetFullUsersInfoFromChannel extends Controller
{
    /**
     * @param Request $request
     * @return \React\Promise\Promise|PromiseInterface|void
     */
    public function __invoke(Request $request)
    {
        return all($this->channelManager
            ->getChannelMembers($request->appId, $request->channelName)
            ->then(function ($members) use ($request) {
                return all([
                    'users' => all(collect($members)->map(function ($user) use ($request) {
                        $this->channelManager->getMemberSockets($user->user_id, $request->appId, $request->channelName)
                            ->then(function ($sockets) use ($user) {
                                $user->sockets = $sockets; //resolved
                                return $user;
                            });
                        return $user; //resolved
                    })->values()->toArray())]);
            }));
    }
}

and

'custom_handlers' => [

         '/apps/{appId}/userinfo/{channelName}' => ['get' , \App\API\GetFullUsersInfoFromChannel::class],

    ],

can't be done in the AppServiceProvider context

@rennokki rennokki closed this Jan 23, 2021
@rennokki rennokki deleted the branch beyondcode:master January 23, 2021 15:09
@simonbuehler
Copy link
Contributor Author

hi,

was this rejected or closed by mistake?

@rennokki
Copy link
Collaborator

rennokki commented Feb 4, 2021

2.x got closed

@rennokki rennokki reopened this Feb 4, 2021
@rennokki rennokki changed the base branch from 2.x to master February 4, 2021 16:16
@rennokki
Copy link
Collaborator

rennokki commented Feb 4, 2021

As far as I checked out, there is a counter-pattern on your implementation, since you're trying to resolve the sync module in async by resolving all the promises.

@simonbuehler
Copy link
Contributor Author

i first tried to make the FullfilledPromise JsonSeralizable but the reactphp guys in my PR lead me to this solution which works fine here.

What would you suggest?

@simonbuehler
Copy link
Contributor Author

this would basically re-enable the lost functionality of #150

@rennokki rennokki closed this Apr 6, 2021
@rennokki
Copy link
Collaborator

rennokki commented Apr 6, 2021

Please see #735, it was a bit off to find out that custom routes accidentally got registered at runtime anytime the kernel ran, not just when the websocket command ran.

@rennokki
Copy link
Collaborator

rennokki commented Apr 6, 2021

@simonbuehler The only difference is that another function may be used to differ from the registration ones:

WebSocketsRouter::addCustomRoute('GET', '/my-websocket', \App\MyCustomWebSocketHandler::class);

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[2.x] Add Custom API Route Handlers
3 participants