Wiredcraft / test-fullstack

6 stars 43 forks source link

complete the test #42

Closed cieldon32 closed 4 years ago

cieldon32 commented 4 years ago

I have write a version of SSR, the link : https://github.com/cieldon32/test-fullstack/tree/daily/0.0.1

flyingant commented 4 years ago

@cieldon32 Hi, this is MaYi from Wiredcraft. And I reviewed your code (mainly frontend).

Just be curious.

I noticed that you have some code related to the Authentication flow in the file (https://github.com/Wiredcraft/test-fullstack/pull/42/files#diff-1970dda743901e6cf68f156ca293072e)

Are you able to explain what's the Auth0Context and how it works?

cieldon32 commented 4 years ago

https://github.com/cieldon32/test-fullstack/blob/daily/0.0.2/web/src/index.provider.tsx

This file is copy from my other application. that will provide the global state for the site.

but I think the test application not need, it . so I modify it , but not cleaned up.

My auth strategy is https://github.com/cieldon32/test-fullstack/blob/daily/0.0.2/server/src/auth/jwt.strategy.ts

Why do I let the server handle the web authentication?

because, I write the SSR version: https://github.com/cieldon32/test-fullstack/blob/daily/0.0.1/src/auth/jwt.strategy.ts

And I read the user story. My understanding is that users who are not logged in can see the talks list. So I let the post a poll need with a token.

Liu Cheng a.k.a MaYi notifications@github.com 于2020年2月10日周一 下午10:00写道:

@cieldon32 https://github.com/cieldon32 Hi, this is MaYi from Wiredcraft. And I reviewed your code (mainly frontend).

Just be curious.

I noticed that you have some code related to the Authentication flow in the file ( https://github.com/Wiredcraft/test-fullstack/pull/42/files#diff-1970dda743901e6cf68f156ca293072e )

Are you able to explain what's the Auth0Context and how it works?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Wiredcraft/test-fullstack/pull/42?email_source=notifications&email_token=AEOZFGP6Z7VZQYI7QCQUIALRCFMWHA5CNFSM4KRHPTP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELITHVI#issuecomment-584135637, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEOZFGKIY6B5HW6WYO5IGOTRCFMWHANCNFSM4KRHPTPQ .

cieldon32 commented 4 years ago

This is a mistake. I should repeat ‘BrowserRouter’ to ‘HashRouter’.

Haishan notifications@github.com 于2020年2月11日周二 上午10:53写道:

@haishanh commented on this pull request.

In web/src/App.tsx https://github.com/Wiredcraft/test-fullstack/pull/42#discussion_r377427153 :

@@ -0,0 +1,26 @@ +import React from 'react'; +import { Switch, Route, HashRouter } from 'react-router-dom'; +import Layout from './components/layout'; +import Home from './containers/home'; +import Login from './containers/login'; +import EditPoll from './containers/editPoll'; +import Register from './containers/register'; + +function App(): JSX.Element { +

  • return (

I see BrowserRouter used web/src/index.tsx

Why do we need HashRouter here?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Wiredcraft/test-fullstack/pull/42?email_source=notifications&email_token=AEOZFGJCWOXWXN2DR3YULI3RCIHLPA5CNFSM4KRHPTP2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCU7CWRY#pullrequestreview-356395847, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEOZFGPZXYFIQ2NZMP3SFRDRCIHLPANCNFSM4KRHPTPQ .

haishanh commented 4 years ago

Thanks for the PR, sorry for nitpicking.

cieldon32 commented 4 years ago

I have fixed these bugs.

best regards carly xia

Haishan notifications@github.com 于2020年2月11日周二 下午5:35写道:

Thanks for the PR, sorry for nitpicking.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Wiredcraft/test-fullstack/pull/42?email_source=notifications&email_token=AEOZFGMAPCVPTVCMNOLRM5DRCJWMXA5CNFSM4KRHPTP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELLXTCA#issuecomment-584546696, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEOZFGOC2RSZAXAYDYPOVJLRCJWMXANCNFSM4KRHPTPQ .

makara commented 4 years ago

Thanks. This should be enough for us to make a decision. We'll get back to you soon. Closing.