Switch to ipinfo.io-based country detection fixing Issue 2448 #2605
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
anth-volk
left a comment
anth-volk
left a comment
There was a problem hiding this comment.
Hi @HerveRI, I've requested a series of changes on this. I've also set up IPInfo for us and added the token to GitHub, so the next time this launches, we should have access to it. We'll have to later confirm that we have access via any Docker images we launch.
src/routing/RedirectToCountry.jsx
Outdated
| const browserLanguage = navigator.language; | ||
| useEffect(() => { | ||
| let cancelled = false; | ||
| getCountryId().then((id) => { |
There was a problem hiding this comment.
nit: Please use async/await over thenables
Thenables are an older standard and async/await is easier to parse and work with. To use it in the context of an effect hook, you can define an async/await function within the hook, then call it and execute it.
src/utils/ipinfoCountry.js
Outdated
| const token = process.env.REACT_APP_IPINFO_TOKEN; | ||
| const resp = await fetch(`https://api.ipinfo.io/lite/me?token=${token}`); | ||
| //const resp = await fetch(`https://ipinfo.io/json?token=${token}`); //in case we ignore ipinfo token | ||
| if (resp.ok) { | ||
| //const { country: iso2 = "" } = await resp.json(); //in case we ignore ipinfo token | ||
| const { country_code: iso2 = "" } = await resp.json(); | ||
| const ISO2 = iso2.toUpperCase(); | ||
|
|
||
| // Return the country-id based on the ISO2 code | ||
| if (["GB", "IM", "JE", "GG"].includes(ISO2)) return "uk"; | ||
| if (ISO2 === "US") return "us"; | ||
| if (ISO2 === "CA") return "ca"; | ||
| if (ISO2 === "NG") return "ng"; | ||
| if (ISO2 === "IL") return "il"; | ||
| } | ||
| } catch (err) { | ||
| console.error("ipinfo lookup failed:", err); | ||
| } |
There was a problem hiding this comment.
issue: When working through a fallback waterfall, please define each option as its own function
So in this case, there'd be a function for finding via IP address, then one via language map, then the overarching function could just return "us"
github-project-automation
bot
moved this from Todo
to PR: Review Requested
in policyengine-app
anth-volk
left a comment
anth-volk
left a comment
There was a problem hiding this comment.
Hey @HerveRI, thanks for your work on this. It looks a lot better, but there are two overriding issues:
- The fallback waterfall is broken
- Your fork is unable to access the IPInfo token
If you could reopen this from a branch (I'll increase your permissions as needed) and modify the fallbacks, that would be great!
src/routing/RedirectToCountry.jsx
Outdated
| } catch (err) { | ||
| if (signal.aborted) return; | ||
| console.error("IP⇢country lookup failed:", err); | ||
| setCountryId("us"); |
There was a problem hiding this comment.
issue, blocking: Please use the navigator language as part of this code.
Previously, I think you had the right idea with the fallback waterfall. I believe this iteration of the code does has two core errors that don't allow for the waterfall:
- This line here sets country ID to "us" no matter how the IP-based lookup fails
- The line I'll point out in ipinfoCountry.js throws on error, preventing any further function execution
I'd simplify this a bit more. In this effect hook, I'd just call getCountryId and let it handle errors. I'd modify getCountryId to call resovleCountryFromIp. On error, don't throw, just console.error and move onto resolveCountryFromLanguage. If that fails, return "us" as country. JS will automatically wrap the sync values in a resolving promise.
| * @returns {Promise<CountryId | null>} | ||
| */ | ||
| export async function resolveCountryFromIp(signal) { | ||
| const token = process.env.REACT_APP_IPINFO_TOKEN; |
There was a problem hiding this comment.
issue, blocking: Please reopen this PR off of a branch, not a fork
As a fork contributor, I believe you don't have access to this environment variable, and I'm sorry that I forgot to advise you on this. Could you reopen this PR off of a branch? Let me know if you need additional permissions to do so.
HerveRI
force-pushed
the
herveRI/issue2448
branch
from
50ec028 to
dc30a6d
Compare
|
Closing in favor of #2681 |
github-project-automation
bot
moved this from PR: Review Requested
to Closed
in policyengine-app
2 participants
Fixes #2448
Description
Replaces the browser-language–based country detection with an IP-based lookup using ipinfo.io, eliminating frequent mis-localisations
Changes
Removed the “guess the visitor’s country from their browser-language” and replaced it with the IP lookup from ipinfo.io. On the very first page load the app pings https://api.ipinfo.io/json with a custon token (which could be changed) reads the two-letter country code, translates that into the country IDs we use internally (“uk”, “us”, “ca”, etc.), and redirects from the root (/) to /{countryId}. If the IP call fails—for example the user is offline or a tracker-blocker gets in the way— fall back to the old language mapping and finally default to “us”.