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

ACC-16 Разлогин/Завершение сессии #17

Conversation

dmi-ch
Copy link
Contributor

@dmi-ch dmi-ch commented Jun 18, 2021

PullRequest checklist

Please, review this checklist, check your work to compliance and mark checkboxes as completed

Branch name

  • Your branch should be prefixed with feature/ or fix/ or chore/ or refactor/ or tests/
  • Add issue number after prefix
  • Add 3-5 words of description separated with -

Example:

feat/12-add-login-form
fix/8-loading-panel-on-first-load
chore/22-add-release-drafter

PullRequest

  • This PullRequest implements new feature, fix bug, or some other changes
  • If PR is not ready mark it as Draft, press "Ready to review" before assigning reviewer
  • Link PullRequest with issue (re #123 just to link, closes #123 or fixes #123 to close)
  • All commits in this PR should be created by yarn commit by conventional-commits

  • I've read checklist and complete all requirements

@linear
Copy link

linear bot commented Jun 18, 2021

ACC-16 Разлогин/Завершение сессии

@dmi-ch dmi-ch force-pushed the feature/acc-16-разлогинзавершение-сессии branch from 348b752 to 4a913b5 Compare June 19, 2021 13:59
@dmi-ch dmi-ch marked this pull request as ready for review June 19, 2021 14:01
@dmi-ch dmi-ch requested review from sergeysova and azinit June 19, 2021 14:02
@azinit
Copy link

azinit commented Jun 19, 2021

image

А-ой, тольк щас увидел нейминг веток)

azinit
azinit previously approved these changes Jun 19, 2021
Copy link

@azinit azinit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вроде ок, тольк пару вопросов есть)

Comment on lines 4 to 5
import * as api from '../../api';
import { sessionDelete } from '../../api';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Как будто что-то пошло не так в импортах)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

да, пропустил, поправлю) на аксессо везде вроде через api. написано

Comment on lines +11 to +15
logoutClicked: model.logout.prepend(noop),
},
});

function noop(): void {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А можешь поделиться пож, что за хак такой с noop ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

там у logoutClicked и model.logout разные payload по сути это трансформер одного payload в другой, в нашем случае просто пустой)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А поч именно на основании model.logout решил сделать?

Это же event обычный так понимаю?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это же event обычный так понимаю?

да

А поч именно на основании model.logout решил сделать?

ну я не придумал как для сессии его по другому назвать) unauthorize тоже странно, а delete на сессии будет лучше, но смущает что там есть еще deleteFx(fetch на удаление) и начинаешь запутываться, но по идее будет лучше

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

а для модели home logout нормально в целом выглядит типа - разлогинься из дома

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я просто пытаюсь понять, какой плюс, что ты одно событие на основании другого делаешь

Тип когда будет срабатывать событие logout- будет всегда срабатывать и событие logoutClicked?

Так ли тогда нужно отдельное событие с prepend?

Comment on lines +12 to +13
export const logoutClicked = createEvent<React.MouseEvent<HTMLButtonElement>>();

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А событие точно в UI должно создаваться? Мб на уровень модели вынести?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

конкретно в этом случае возможно, думал, что может еще придется менять и еще что то по клику делать, в целом оно все равно биндится в контракте

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну я бы постарался не раздувать UI-сегмент такими штуками из модели)

Но решать тебе офк

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

предлагаешь прокинуть напрямую из модели? через bind на contract ? его придется просто класть в модели как event от mouse click, что кажется странным, типа зачем модели знать про какие то клики

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

типа зачем модели знать про какие то клики

Точно так же можно и сказать "Зачем UI знать про какие-то createEvent")))

Я бы все события / БЛ объявлял в модели (привет фиче-слайсам) , а в UI лишь переиспользовал их в инкапсулированном виде, не залезая в реализацию

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вот здесь Сова тож хорошо про "инкапсулированность" БЛ от UI внутри модели раскрывает

https://t.me/sergeysova/88

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Крч я не особо против такого подхода, просто пытаюсь понять причины изначальные)

Тип если не хватает текущих событий (а там вродь как event.logout есть) - то можно создать рядом еще одно и его переюзать)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Чтобы страница не импортировала модель

Comment on lines +67 to +76
const Failure = reflect({
view: ({ showError }: { showError: boolean }) =>
showError ? (
<ErrorText>Something went wrong! Please, try again later</ErrorText>
) : null,
bind: {
showError: $showError,
},
});

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Так понимаю в effector принято так писать, хотя и выглядит для меня как blackMagic + overengineering))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

я сам не мастер, в accesson reflect уважается, я смотрел немного исходники, по сути просто биндинги под капот reflect спрятали, из-за этого отрезался api но упростился вид, если нужен полный api то придется все равно по старинке писать)

Comment on lines +24 to +31
sample({
source: guard({
source: logout,
filter: sessionDeleteFx.pending.map((is) => !is),
}),
target: sessionDeleteFx,
fn: (_) => ({ body: { deleteAllSessions: true } }),
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А можешь пож расписать, что тут происходит?

В эффекторе ньюфаг пока)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не смотря на то что, Sample cчитается "выборкой" со специальной семантикой, я предпочитаю смотреть на него как расширенный forward(from, to) который по сути перекидывает event из одного места в другое, но с настройками на частоту обновления и возможность модификации данных,
Guard просто работает как фильтр
Как итог получается получается - мы берем событие logout(которое триггерит клик), не отсылаем его пока эффект(запрос) находится в pending и пересылаем его в эффект(запрос), с хардкодными данными(в будущем видимо чекбокс будет)
То есть такой throttle/debounce по условию, чтобы лишние клики просто игнорить)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Спс за разъяснение)

Тогда да, рили работает примерно как думал

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pending.map((is) => !is),

А тут .map это кастомно встроенный обработчик (не Array.prototype.map), который позволяет смаппить во что-то состояние эффекта? Или что-то другое?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

да это по сути store.map т.к. pending это Store

@azinit
Copy link

azinit commented Jun 19, 2021

@dmi-ch А можешь еще пож в двух словах описать - для чего accesso-app/frontend ?

Он именно про то самое "Окно с формами авторизации / регистрации Accesso" , которое в проектах будет открываться?

@dmi-ch
Copy link
Contributor Author

dmi-ch commented Jun 19, 2021

Он именно про то самое "Окно с формами авторизации / регистрации Accesso" , которое в проектах будет открываться?

Да, регистрацию авторизацию и восстановление паролей уже Сергей сделал, это уже конкретно домашняя страничка, по идее как то будет список приложений еще(или только в админке, я точно не знаю), там еще отдельно будет редирект видимо на нужную страничку

@sergeysova sergeysova merged commit dc77178 into master Jun 24, 2021
@sergeysova sergeysova deleted the feature/acc-16-разлогинзавершение-сессии branch June 24, 2021 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants