-
Notifications
You must be signed in to change notification settings - Fork 6
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
some fixes #16
base: master
Are you sure you want to change the base?
some fixes #16
Conversation
if (!this.params.ci) { | ||
console.error(`Some connections was not completed, but navigation happened.`) | ||
console.error(`That is bad, mkay? Because you have a race: server response and navigation`) | ||
console.error(`That will lead to heisenberg MONOFO errors in case when response will win the race`) | ||
console.error(`Alive connections:\n${fy([...this.reqSet])}`) | ||
throw new Error(`Some connections was not completed, but navigation happened.`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Эта штука тут специально, потому что если у тебя есть активные запросы и ты в этот момент уходишь на другую страницу — что-то пошло не так.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Так мы и так увидим комментов кучу по этому поводу + список запросов - нафиг нам еще и throw тут? который кстати не кэтчится и оттого спама много - смысла ноль
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Смысл в том, чтоб всё упало, и съёмщик моков поправил код. На логи все хер забили.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Так оно и не падает) бессмысленый throw выходит. Могу его переделать попозже в более лучшем виде - сейчас он, ну такое...
Давай верну если он так дорог
_onSetReqCache: ({ url = '', method, postData }) => { | ||
const { protocol, hostname, pathname } = new URL(url) | ||
|
||
this.cachedReqs.set(`${protocol}//${hostname}${pathname}_${method}`, postData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А чем это отличается от url? Двоеточие убрал? Зачем?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
от урла отличается тем - что сюда не пишется search - который нафиг не нужен - с ним не удобно - так как тогда нам надо искать именно весь урл - и знать его search параметры - это лишнее. Двоеточие не надо, так как protocol отдается как https:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Дак а если у тебя два запроса будет но с разными query параметрами, ты их будешь кешировать в одно место?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да ладно - когда такое надо было то? ну ок. Соглашусь
No description provided.