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

Apple captive portal check #72

Closed
jish opened this issue May 3, 2020 · 2 comments · Fixed by #73
Closed

Apple captive portal check #72

jish opened this issue May 3, 2020 · 2 comments · Fixed by #73

Comments

@jish
Copy link

jish commented May 3, 2020

Hello,

I was reading through your readme and I was curious what you meant by "Retrieve Apple's Captive Portal test page" https://github.com/sindresorhus/is-online/blob/936f3fe354fee3e3bfa80874263e292c40b97524/readme.md#how-it-works

So, I checked in the source code

is-online/index.js

Lines 19 to 20 in 936f3fe

if (body && body.includes('Success')) {
throw new Error('Apple check failed');

It looks like, if the string "Success" is present then a new Error is thrown. I'm confused how the success case works for this captive portal request 🤔 Could you help me understand?

Do you think it would be helpful to add some documentation around the Apple Captive Portal test page?

@sindresorhus
Copy link
Owner

I think this is a bug that was introduced in 0349077.

// @LuKks

@LuKks
Copy link

LuKks commented May 4, 2020

Could you help me understand?

The success check was in the opposite direction:

if (body && body.includes('Success')) {

->

if (!body || !body.includes('Success')) {

Do you think it would be helpful to add some documentation around the Apple Captive Portal test page?

No no, it was just a wrong condition.
It would be awesome if the test checks for the resulted boolean (which should be true), because I ran tests before submitting the pull request that caused the bug and everything was ok.
Also, silly of me that didn't see that little wrong condition, I think I was moving some code and I didn't notice it.

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 a pull request may close this issue.

3 participants