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

Lesson 12: Routing #21

Merged
merged 16 commits into from
May 15, 2020
Merged

Lesson 12: Routing #21

merged 16 commits into from
May 15, 2020

Conversation

vvscode
Copy link
Collaborator

@vvscode vvscode commented May 13, 2020

No description provided.

src/App.tsx Outdated

export const App: React.FC<{}> = () => (
<Router>
<div>
Copy link
Owner

Choose a reason for hiding this comment

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

Is is redundant <div> ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

пример из доки )) убираю

</Route>
</Switch>
</div>
</Router>
Copy link
Owner

Choose a reason for hiding this comment

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

Router вполне себе родитель вернего уровня, на первый взгляд

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

да, просто некоторые контекст-обертки требуют только одного ребенка

@@ -0,0 +1,15 @@
import { sleep } from '@/utils/sleep';

export const login = async (name: string) => {
Copy link
Owner

Choose a reason for hiding this comment

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

Может в login/logout тоже добавить задержку?

name: string;
}

class RawUserScreen extends React.PureComponent<
Copy link
Owner

Choose a reason for hiding this comment

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

memo instead of PureComponent ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Тут специально сделан класс (логин сделал через FC), чтобы были оба пример работы с роутом

name: string;
}
const Component: React.FC<ComponentProps> = ({ name }) => <h1>{name}</h1>;
let WrappedComponent: React.ComponentType<ComponentProps>;
Copy link
Owner

Choose a reason for hiding this comment

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

может использовать везде const? не прочитал с первого раза

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

там был let из-за beforeEach

failed,
}

export const authorizedOnlyHoc = <Props extends object>(
Copy link
Owner

Choose a reason for hiding this comment

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

Ох, может использовать класс? Хуки это страдание...

Copy link
Owner

Choose a reason for hiding this comment

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

ИМХО, разумеется =)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Я согласен, но судя по проверке домашек - на хуках народ пишет гораздо чаще ))

я могу дать для практики задание переписать это на класс, благо тесты уже есть =)

const Component: React.FC<ComponentProps> = ({ name }) => <h1>{name}</h1>;
let WrappedComponent: React.ComponentType<ComponentProps>;
beforeEach(() => {
WrappedComponent = authorizedOnlyHoc(Component);
Copy link
Owner

Choose a reason for hiding this comment

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

Я не понял, что тут произошло, можешь пояснить, плиз?
Почему нельзя использовать один компонент в двух тестах?

Copy link
Owner

Choose a reason for hiding this comment

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

Если надо 2 разных компонента, предлагаю использовать явное объявление в тестах, вместо неявного сброса

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

можно, но почему же сброс не явный?

тут с одной стороны вкусовщина, с другой - это в своей реализации я знаю, что в хоке нет сайд эффектов и я могу использовать свой компонент дважды, а в целом это не гарантированно, так что лучше создать новый компонент

а если создавать одно и то-же -- то beforeEach как раз для этого

и может у меня избирательное внимание, но я этот подход использую и вижу вокруг
https://grischuk.de/how-to-test-react-component-wrapped-in-with-router-hoc
enzymejs/enzyme#711
https://medium.com/@kacperwdowik/testing-simple-as-implementing-how-to-test-higher-order-components-with-jest-and-enzyme-da4abe01f965

Copy link
Owner

Choose a reason for hiding this comment

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

Могу поделиться мнением) Библиотеки смертны, подходы меняются, объявить отдельную переменную во всех языка вечное
А если без философии, объявить переменную в скоупе функции действительно очевиднее, с какой стороны не посмотри.
Если выбирать между двумя переменными и beforeEach я бы выбрал две переменные

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

а я beforeEach ибо DRY =)

плюс setup/teardown это классика тестовых фреймворков

в любом случае убрал

@nickovchinnikov
Copy link
Owner

nickovchinnikov commented May 14, 2020

Так же обновить Readme, знаю что ты помнишь, но я все же оставлю этот коммент тут =)

@vvscode vvscode merged commit c5b3ff0 into master May 15, 2020
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.

None yet

2 participants