IncomingRequestCfPropertiesGeographicInformation makes geo property access an error (regression from 3.17.0)

[maxshirshin]

IncomingRequestCfPropertiesGeographicInformation (as part of the IncomingRequestCfProperties union type) is defined in such a way that it treats accessing of geo fields on the Request instances .cf field as an error.

Here's a Typescript Playground Link for an isolated case that demonstrates the issue.

This makes e.g. this code to raise a type error:

/* request is a Request instance */
console.log(request.cf?.city)

Error: Property 'city' does not exist on type 'IncomingRequestCfProperties<unknown>'. Expectation: read access to any geo property shouldn't be treated as a type error.

This is a regression from v3.17.0 where it worked fine, and is likely related to this change.

[philipatkinson]

I have a pull request that should fix this: #310.

[Qantas94Heavy]

Technically you can get the original to work if you perform an in check to narrow the type down, e.g.

if (req.cf && 'city' in req.cf) {
  console.log(req.cf.city);
  console.log(req.cf.continent);
}

However this can be sometimes unwieldy, especially given ?. is not used for type narrowing.

[caass]

I think the new types are technically correct -- the way that the geographic information is gathered in Cloudflare's edge, there really may not be a city property. Is there any downside to @Qantas94Heavy's suggestion of using city in req.cf for type narrowing? You could potentially write a function like

function hasGeographicInformation(cf: IncomingRequestCfPropertiesGeographicInformation): city is Extract<IncomingRequestCfProperties, { country: Iso3166Alpha2Code }> {
  return "country" in cf && cf.country !== "T1"
}

// works!
if (hasGeographicInformation(cf)) {
  console.log(cf.city)
}

Playground link

[philipatkinson]

I guess this is question of whether it is reasonable to support optional chaining on the (possible) cf property or not? My pull request was ultimately because I do use optional chaining (rather than the property check method noted above). Seems that both are technically valid?

[Qantas94Heavy]

While the type is technically valid, the change did break existing code in a minor version upgrade (without a breaking change in the actual Workers runtime). As such I'd support allowing optional chaining again and leaving any discussion of changing this to V4.

[caass]

Yeah, I see the issue. I wonder if there's a way to have the type be both semantically correct and also allow optional chaining...

[lostpebble]

This is quite a big regression and the solutions are very unwieldy.

This is one of those niggles of TypeScript for me- when you are creating a large union type with | (or) - you need to actually include all the properties in each object type (just mark them as { city?: undefined } if they are undefined in a specific form). Or have an enum propery we can check against to discriminate between them.

If TypeScript can't know for sure that a property within an object exists (even if it exists in one of the subsets of the union types), it will throw this error we see here.

Yeah, I see the issue. I wonder if there's a way to have the type be both semantically correct and also allow optional chaining...

I think doing what I suggested above (actually naming the properties but making them ?: undefined) should work...