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

[Examples] Use next-connect in the with-passport example #11747

Merged
merged 1 commit into from
Apr 13, 2020

Conversation

lfades
Copy link
Member

@lfades lfades commented Apr 8, 2020

Express.js doesn't have to be used for serverless functions, next-connect looks like a better, more lightweight alternative.

@hoangvvo Thank you for next-connect 💯

@lfades lfades requested a review from Timer as a code owner April 8, 2020 09:26
@ijjk
Copy link
Member

ijjk commented Apr 8, 2020

Stats from current PR

Default Server Mode
General
zeit/next.js canary lfades/next.js example-fix/with-passport Change
buildDuration 11.6s 11.8s ⚠️ +160ms
nodeModulesSize 52.5 MB 52.5 MB
Client Bundles (main, webpack, commons)
zeit/next.js canary lfades/next.js example-fix/with-passport Change
main-HASH.js gzip 6.25 kB 6.25 kB
webpack-HASH.js gzip 746 B 746 B
de003c3a9d30..e3d8.js gzip 10.2 kB 10.2 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
Overall change 56.3 kB 56.3 kB
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary lfades/next.js example-fix/with-passport Change
main-HASH.module.js gzip 4.78 kB 4.78 kB
webpack-HASH..dule.js gzip 746 B 746 B
de003c3a9d30..dule.js gzip 6.78 kB 6.78 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
Overall change 51.5 kB 51.5 kB
Legacy Client Bundles (polyfills)
zeit/next.js canary lfades/next.js example-fix/with-passport Change
polyfills-HASH.js gzip 26.3 kB 26.3 kB
Overall change 26.3 kB 26.3 kB
Client Pages
zeit/next.js canary lfades/next.js example-fix/with-passport Change
_app.js gzip 1.24 kB 1.24 kB
_error.js gzip 3.15 kB 3.15 kB
hooks.js gzip 664 B 664 B
index.js gzip 222 B 222 B
link.js gzip 2.03 kB 2.03 kB
routerDirect.js gzip 279 B 279 B
withRouter.js gzip 278 B 278 B
Overall change 7.86 kB 7.86 kB
Client Pages Modern
zeit/next.js canary lfades/next.js example-fix/with-passport Change
_app.module.js gzip 594 B 594 B
_error.module.js gzip 2.08 kB 2.08 kB
hooks.module.js gzip 370 B 370 B
index.module.js gzip 212 B 212 B
link.module.js gzip 1.48 kB 1.48 kB
routerDirect..dule.js gzip 271 B 271 B
withRouter.m..dule.js gzip 270 B 270 B
Overall change 5.28 kB 5.28 kB
Client Build Manifests
zeit/next.js canary lfades/next.js example-fix/with-passport Change
_buildManifest.js gzip 61 B 61 B
_buildManife..dule.js gzip 61 B 61 B
Overall change 122 B 122 B
Rendered Page Sizes
zeit/next.js canary lfades/next.js example-fix/with-passport Change
index.html gzip 918 B 918 B
link.html gzip 926 B 926 B
withRouter.html gzip 913 B 913 B
Overall change 2.76 kB 2.76 kB

Serverless Mode (Increase detected ⚠️)
General
zeit/next.js canary lfades/next.js example-fix/with-passport Change
buildDuration 12.6s 12.2s -330ms
nodeModulesSize 52.5 MB 52.5 MB
Client Bundles (main, webpack, commons)
zeit/next.js canary lfades/next.js example-fix/with-passport Change
main-HASH.js gzip 6.25 kB 6.25 kB
webpack-HASH.js gzip 746 B 746 B
de003c3a9d30..e3d8.js gzip 10.2 kB 10.2 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
Overall change 56.3 kB 56.3 kB
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary lfades/next.js example-fix/with-passport Change
main-HASH.module.js gzip 4.78 kB 4.78 kB
webpack-HASH..dule.js gzip 746 B 746 B
de003c3a9d30..dule.js gzip 6.78 kB 6.78 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
Overall change 51.5 kB 51.5 kB
Legacy Client Bundles (polyfills)
zeit/next.js canary lfades/next.js example-fix/with-passport Change
polyfills-HASH.js gzip 26.3 kB 26.3 kB
Overall change 26.3 kB 26.3 kB
Client Pages
zeit/next.js canary lfades/next.js example-fix/with-passport Change
_app.js gzip 1.24 kB 1.24 kB
_error.js gzip 3.15 kB 3.15 kB
hooks.js gzip 664 B 664 B
index.js gzip 222 B 222 B
link.js gzip 2.03 kB 2.03 kB
routerDirect.js gzip 279 B 279 B
withRouter.js gzip 278 B 278 B
Overall change 7.86 kB 7.86 kB
Client Pages Modern
zeit/next.js canary lfades/next.js example-fix/with-passport Change
_app.module.js gzip 594 B 594 B
_error.module.js gzip 2.08 kB 2.08 kB
hooks.module.js gzip 370 B 370 B
index.module.js gzip 212 B 212 B
link.module.js gzip 1.48 kB 1.48 kB
routerDirect..dule.js gzip 271 B 271 B
withRouter.m..dule.js gzip 270 B 270 B
Overall change 5.28 kB 5.28 kB
Client Build Manifests
zeit/next.js canary lfades/next.js example-fix/with-passport Change
_buildManifest.js gzip 61 B 61 B
_buildManife..dule.js gzip 61 B 61 B
Overall change 122 B 122 B
Serverless bundles Overall increase ⚠️
zeit/next.js canary lfades/next.js example-fix/with-passport Change
_error.js gzip 233 kB 233 kB -307 B
404.html gzip 1.32 kB 1.32 kB
hooks.html gzip 956 B 956 B
index.js gzip 233 kB 233 kB ⚠️ +208 B
link.js gzip 243 kB 243 kB -2 B
routerDirect.js gzip 241 kB 241 kB ⚠️ +68 B
withRouter.js gzip 241 kB 241 kB ⚠️ +50 B
Overall change 1.19 MB 1.19 MB ⚠️ +17 B

@hoangvvo
Copy link
Contributor

hoangvvo commented Apr 8, 2020

Wow, I never dreamed about my library getting used in an official example 😨. I actually refrain from doing something like to such an example because it sounds kinda "bias/self-promotion".

I would make some changes:

  • The user log out by being redirected to api/logout.js. This is not needed. We can refactor this with a fetch to /api/logout, so the user never leaves the web app.
  • api/login.js is not really conventional for passport. We can refactor it into something like this

Thanks!

@lfades
Copy link
Member Author

lfades commented Apr 8, 2020

@hoangvvo Thank you for your feedback, here are my thoughts:

  • The first case was the simplest way of doing a logout, it also helps for cases where restarting the app state is required (because it's basically a reload), can it be just a fetch, yes, but it's not made that way as it was more complicated.
  • I'm not a Passport expert but I don't like their middlewares and overall methods, the way it's done feels better for the app as it has less passport implementation details. That's my opinion, you're welcome to disagree and show me a better solution.

@lfades lfades closed this Apr 8, 2020
@lfades lfades reopened this Apr 8, 2020
@hoangvvo
Copy link
Contributor

hoangvvo commented Apr 8, 2020

@lfades I agree with your second point. I just feel like it would be cleaner otherwise, but after all, it is really subjective. The less passport implementation details makes sense. I do not like Passport.js API either. It overcomplicates auth while its doc is terrible 😨. Doing auth without it results in even fewer lines of code. Yet, people ore obsessed with passport and would say any implementation without it is "reinvent the wheel" and = bad

On the first point, I think the way it is right now is fine. Yet, if you are to change your mind, a fetch can be implemented simply with https://github.com/zeit/next.js/blob/3a4c5b3572668c5c923323df2e860eb87fd73fa8/examples/with-passport-and-next-connect/components/Navbar.js#L7-L10

We would simply call useSWR's mutate to reset the state.

Either way, I left some comments on #11359. Basically the other PR is almost similar in term of functionality. Since the other one now uses @hapi/iron and this also uses next-connect, we can combine the two together.

@lfades
Copy link
Member Author

lfades commented Apr 13, 2020

I'll leave both examples for the moment, as they currently show different usages of Passport.js, also this one will always be the minimal working implementation of Passport.js for serverless functions (it was designed for servers, and not really with Promises in mind), if at any point I find a better alternative to next-connect I will update it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants