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

Use of process.nextTick() at db/users.js #23

Open
jilvin opened this issue Mar 19, 2020 · 16 comments
Open

Use of process.nextTick() at db/users.js #23

jilvin opened this issue Mar 19, 2020 · 16 comments

Comments

@jilvin
Copy link

jilvin commented Mar 19, 2020

@jaredhanson Is the use of process.nextTick() at db/users.js necessary?

It seems you have clarified the addition of nextTick on a stack overflow question.
https://stackoverflow.com/questions/20743348/passport-js-and-process-nexttick-in-strategy

Maybe we should add a comment stating the reason of usage.

A lot has changed regarding the execution of nextTick since Node v12. Should we shift to the use of setImmediate()(since nextTick gets executed immediately in the current phase itself of the event loop)?

PS: I am a beginner. I am pondering around to improve my knowledge on the subject. Cheers.

@Xalior
Copy link

Xalior commented Mar 19, 2020

There is a comment. Perhaps it is not clear to you :) but to me it’s saying it’s “faking an async action to verify it works in all cases”.

Hope that helps explain the use logic.

@jilvin
Copy link
Author

jilvin commented Mar 19, 2020

@Xalior I was thinking maybe we could add some comment lines in code where it is used to make it clear to everyone including beginners like me.

@Xalior
Copy link

Xalior commented Mar 19, 2020

shrug is it the purpose of this repo to teach node generics?

How verbose a comment should be is open to endless debate, and fundamentally the choice of the comment writer :-)

@jilvin
Copy link
Author

jilvin commented Mar 19, 2020 via email

@jilvin
Copy link
Author

jilvin commented Mar 19, 2020

@Xalior I understand that it's pretty much the basics of Node.js. The issue is that most developers(including me) are not aware of constructs like process.nextTick(). It would be helpful if we can add a one line comment to help clarify the subject.

@Vincent440
Copy link

I can't agree more that some clarification would of saved me hours of dissecting that code months ago.

One comment to suggest what was happening there would of been a great improvement to what is supposed to be an example repository.

@Xalior
Copy link

Xalior commented Apr 14, 2020 via email

@jilvin
Copy link
Author

jilvin commented Apr 14, 2020

@Xalior It seems the behavior of process.nextTick() has changed since node v11. I was thinking of getting more knowledge on the usage of the subject from production level developers, that too of a library that is well renown world wide. It is true what you said that more information can be found via a simple google search. But the issue remains that most developers(including the authors of blogs on the web) are unaware or has poor understanding of the intricate working of node.js event loop. I just believed that by adding a good comment section in this part of the code will help enlighten everyone including me. I am not saying we should. It's just a suggestion. Cheers.

nodejs/node#22257
nodejs/node#22842
nodejs/nodejs.org#1804

@Xalior
Copy link

Xalior commented Apr 14, 2020 via email

@jilvin
Copy link
Author

jilvin commented Apr 14, 2020

@Xalior I agree with you that its not the job of a feature, function or framework specific tutorial to teach anyone generic node code. It's just a harmless suggestion to document the code well so that even beginners may grasp the concepts easily and hence lead to better usage of the library itself rather than messing around in the dark. Moreover, there is prevalent unawareness about the event loop within the node.js community. Cheers.

@Xalior
Copy link

Xalior commented Apr 14, 2020 via email

@Vincent440
Copy link

Vincent440 commented Apr 14, 2020

Really? Because googling “what does node next tick do” gives a pretty reasonable answer on the first page... If you’re not able to google and look up one line of code you don’t understand maybe you should stop moaning at free tutorials and pay someone to teach you, because you certainly do not seem to be able to do it alone!

Sent from my iPhone
On 14 Apr 2020, at 03:43, Vincent Shury @.***> wrote:  I can't agree more that some clarification would of saved me hours of dissecting that code months ago. One comment to suggest what was happening there would of been a great improvement to what is supposed to be an example repository. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

Get off open source if you are going to tell new coders they need to pay to learn more over requesting a single line comment for clarification on an irrelevant piece of code in a module.
you POS.

@jilvin
Copy link
Author

jilvin commented Apr 14, 2020

@Vincent440 Hey. @Xalior shared their opinion on the matter. Don't take it personally. I am sure it was not intentional. And please retract your comment. Let's keep the discussion clean. It's up to the maintainers to decide what to do.

@Xalior
Copy link

Xalior commented Apr 14, 2020 via email

@Vincent440
Copy link

I apologize maybe there was a miscommunication sometime when you attacked me for adding my input, however I do not think your comment was at all appropriate as a response to my input on this issue.

I appreciate the work everyone has done to make Passport and these examples available, I am sorry I tried to add some input honestly.

@VimHax
Copy link

VimHax commented Apr 28, 2020

IMO, process.nextTick is not a widely known feature, so many may wonder why it's there at first glance, a more effective solution could just be putting the code inside a setTimeout, this is much more widely known and clearly let's the user know that the code is async.

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

No branches or pull requests

4 participants