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

Replace readline with message transport #157

Merged
merged 3 commits into from
Aug 24, 2022

Conversation

TiagoSeverino
Copy link
Contributor

@TiagoSeverino TiagoSeverino commented Aug 23, 2022

This change is Reviewable

src/types.ts Outdated Show resolved Hide resolved
src/upload.ts Outdated
});
let code = await rl.question('Enter the code that was sent to you via SMS: ');
if (!messageTransport.prompt) throw new Error('Prompt Method Required to Receive SMS')
let code = await messageTransport.prompt('Enter the code that was sent to you via SMS: ')
Copy link
Contributor

@pierreminiggio pierreminiggio Aug 24, 2022

Choose a reason for hiding this comment

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

  • We're throwing an exception without closing the browser, which will lead to unexpected behaviours (node process still running, maybe chromium processes stackings, etc.). So close that browser before throwing.

  • I think we should handle no code being inputed, either through a try catch, or by allowing the signature to return null (so then we can check if "code" is empty or not).
    We probably want to close the browser and end end the execution of the function if the user doesn't input a code or inputs an empty one.

Example of browser closing + throw if you need one :

if (errorMessage) {
await browser.close()
throw new Error('Youtube returned an error : ' + errorMessage)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gonna add the input and exception handling.

@pierreminiggio
Copy link
Contributor

pierreminiggio commented Aug 24, 2022

@dement6d This MR impacts the changes you've made here : #101.
This means that if this MR gets merged and you update, you'll have to specify your readline callback through the messageTransport parameter.

@pierreminiggio
Copy link
Contributor

pierreminiggio commented Aug 24, 2022

@fawazahmed0 Everything in this MR looks all right to me, I just think the update implementing this MR should be considered a major one when it comes to the version of the package (so 2.0.0), because it will force people who relied on the readline implemented here #101 to implement their own readline callback.

@TiagoSeverino
Copy link
Contributor Author

@fawazahmed0 Everything in this MR looks all right to me, I just think the update implementing this MR should be considered a major one when it comes to the version of the package (so 2.0.0), because it will force people who relied on the readline implemented here #101 to implement their own readline callback.

Maybe we can default onSmsVerificationCodeSent to the old implementation using readline

@pierreminiggio
Copy link
Contributor

pierreminiggio commented Aug 24, 2022

Maybe we can default onSmsVerificationCodeSent to the old implementation using readline

I'm not sure we want that because #153.
By keeping readline, we're basically forcing people not being able to use the LTS version of NodeJS.

@fawazahmed0
Copy link
Owner

Thanks @pierreminiggio and @TiagoSeverino , I will merge this PR. In future I may still add code from Nodejs current version if necessary.

@fawazahmed0 fawazahmed0 merged commit 5cc417f into fawazahmed0:main Aug 24, 2022
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.

3 participants