Conversation
…code to use classes;
…e and id from database;
… is used on forum production; Update ws library;
…mentations; Get user id from Q2A and match it with remaining user/socket-client metadata; Further ESLint fixes;
awaluk
left a comment
awaluk
left a comment
There was a problem hiding this comment.
Jak dla mnie ok, ale najlepiej jakby na kod więcej zerknął ktoś od JSa
d-bend
left a comment
d-bend
left a comment
There was a problem hiding this comment.
Potrzebne są zmiany w części serwera http. Osobiście uważam, że ten refactor wprowadza więcej zamieszania niż rozwiązań. Co do części WS - mogłoby tak zostać jak jest. Proponuje przemyśleć migracje na socket.io, kod powinien się uprościć 5-10 razy w stosunku do tego co jest teraz.
| { | ||
| method: 'all', | ||
| path: '*', | ||
| listener: HttpServer.onAll, | ||
| }, |
There was a problem hiding this comment.
Handler dla nieobsługiwanych route'ów powinien być zarejestrowany jako ostatni żeby uniknąć konfliktów przy routingu.
| static onAll(req, res, next) { | ||
| if (req.headers.token !== config.token) { | ||
| res.sendStatus(HTTP_STATUS_CODES.FORBIDDEN); | ||
| return; | ||
| } | ||
|
|
||
| if (req.method !== 'POST') { | ||
| res.sendStatus(HTTP_STATUS_CODES.METHOD_NOT_ALLOWED); | ||
| return; | ||
| } | ||
|
|
||
| next(); | ||
| } |
There was a problem hiding this comment.
- Łamie SRP. Pierwszy IF jest middleware'em uwierzytelniającym, powinien być zarejestrowany jako app.use i być zarejestrowanym przed pozostałymi handlerami
- Drugi If powinien być osobnym handlerem, w expressie się obsługuje taką sytuacje w inny sposob, np
app.get('/products', ....);
app.post('/products', ...);
// ważna jest kolejność - jest jako ostatni
app.all('/products', (req, res, next) => { res.status(405).send('Method not allowed'); });
| middlewareList.forEach(({ method, path, listener }) => { | ||
| if (!method || !path || !listener) { | ||
| throw ReferenceError(` | ||
| middleware must contain: method, path and listener params! | ||
| Received method: "${method}", path: "${path}", listener: "${listener}" | ||
| `); | ||
| } | ||
|
|
||
| this.app[method](path, listener); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Moim zdaniem to jest przekombinowane, w expressie kolejność middleware/handler-ów ma kluczowe znaczenie. Taki sposób tworzenia serwera wprowadza większe zamieszanie niż z tego jest pożytek.
| }, | ||
| ]); |
There was a problem hiding this comment.
Teoretycznie brakuje handlera dla 404 oraz handlera błędów.
| this.SUBPAGE_VISITORS_AMOUNT_MAP = (() => { | ||
| // create pure object, without prototype | ||
| const visitorsMap = Object.create(null); | ||
|
|
||
| return { | ||
| incrementIfExists(subPageName) { | ||
| if (!(subPageName in visitorsMap)) { | ||
| visitorsMap[subPageName] = 0; | ||
| } | ||
|
|
||
| visitorsMap[subPageName]++; | ||
| }, | ||
| decrementAndEventuallyRemove(subPageName) { | ||
| if (!(subPageName in visitorsMap) || visitorsMap[subPageName] === 0) { | ||
| throw Error(` | ||
| visitorsMap does not have property subPageName or it's value is 0! | ||
| visitorsMap entries: "${Object.entries(visitorsMap)}", subPageName: "${subPageName}" | ||
| `); | ||
| } | ||
|
|
||
| visitorsMap[subPageName]--; | ||
|
|
||
| if (visitorsMap[subPageName] === 0) { | ||
| delete visitorsMap[subPageName]; | ||
| } | ||
| }, | ||
| }; | ||
| })(); |
There was a problem hiding this comment.
W zasadzie to takie coś jest już zaimplementowane i dostępne chociażby z socket.io - https://socket.io/docs/v4/rooms/. Ale to by wymagało też zmian po stronie klienta, więc może na nowym froncie ;)
|
@d-bend dziękuję za code review. Poprawki wprowadzę później, gdy wrócę do tego PR-a. Póki co zmieniłem go na draft. |
3 participants
Częściowo powiązane z: CodersCommunity/forum.pasja-informatyki.local#275
Sesja użytkownika obsługiwana jest poprzez automatyczne wysłanie ciastka z property
qa_sessionw trakcie inicjacji WebSocketu przez przeglądarkę. Node następnie pobiera z Q2A identyfikator użytkownika na podstawie otrzymanego ciastka i dzięki temu może identyfikować, do których użytkowników wysyłać konkretne powiadomienia, które są wysyłane przez Q2A w postaci eventów.Przy okazji zrefaktorowałem trochę kod: serwery HTTP i WebSocket rozdzieliłem na osobne klasy, które reprezentują wysoko i nisko (suffix "Core" w nazwie klasy) poziomowe implementacje. Nie jestem pewien czy wyszło dobrze, ale myślę, że to ułatwi orientację, w którym miejscu jest kod służący do dodawania lub rozszerzania ficzerów, a w którym konfiguracja i inicjacja samego połączenia.
Dodałem też ESLint'a i ustawiłem Babel'a, aby targetem budowania kodu był Node v11.9, którego mamy na produkcji. Dzięki temu m.in. kod
async/awaitnie jest transpilowany (pojawiły mi się błędy z brakiem pollyfilowych generatorów dla wspomnianegoasync/await) i całościowo/distjest mniejszy.