♻️ Use asgiref.typing instead of starlette.types

[br3ndonland]

Description

Resolves #1217

This PR will replace usage of the custom Starlette types with standard ASGI types from the asgiref package.

Changes

Changes are based on the suggestions from @Kludex in #1217 (comment).

• 1. Refactor Message type usage to ASGISendEvent and ASGIReceiveEvent to match asgiref
• 2. Refactor usage of the other types from starlette.types to match asgiref
  • ASGIApp -> ASGI3Application (Starlette 0.12 upgraded to ASGI 3)
  • Receive -> ASGIReceiveCallable
  • Send -> ASGISendCallable
  • Scope -> HTTPScope | LifespanScope | WebSocketScope
• 3. Import types from asgiref
  • Add asgiref>=3.4.0,<4 to setup.py (version matches Uvicorn, but with upper bound added)
  • Update imports: from starlette.types -> from asgiref.typing
  • Fix mypy errors after updating imports (202 errors in 16 files)
  • Deprecate starlette.types
• 4. Fix test failures
  • tests/test_endpoints.py
  • tests/test_requests.py
  • tests/middleware/test_cors.py
  • tests/middleware/test_session.py

Notes

Event and Scope instances

Many of the type errors were resolved by using the asgiref TypedDict classes for event and scope instances, instead of plain dicts.

starlette/starlette/responses.py

Lines 161 to 170 in c3ba37c

	start_event = HTTPResponseStartEvent(
	type="http.response.start",
	status=self.status_code,
	headers=self.raw_headers,
	)
	body_event = HTTPResponseBodyEvent(
	type="http.response.body",
	body=self.body,
	more_body=False,
	)

Type narrowing

There are some assert statements added for type narrowing.

starlette/starlette/endpoints.py

Lines 116 to 122 in c3ba37c

	if message.get("text") is not None:
	text = message["text"]
	assert isinstance(text, str)
	else:
	message_bytes = message.get("bytes")
	assert isinstance(message_bytes, bytes)
	text = message_bytes.decode("utf-8")

In these cases, the type checker thinks the object could be a broader type like object, and the assert helps ensure that the type is correct.

Scope keys

There were a few # type:ignore[index] and # type: ignore[typeddict-item] comments needed when indexing into Scope using keys not present in the asgiref types:

• scope["app"]
• scope["auth"]
• scope["path_params"]
• scope["router"]
• scope["session"]
• scope["state"]
• scope["user"]

starlette/starlette/requests.py

Lines 151 to 156 in c3ba37c

	@property
	def session(self) -> dict:
	assert (
	"session" in self.scope
	), "SessionMiddleware must be installed to access request.session"
	return self.scope["session"] # type: ignore[typeddict-item]

In a future PR, we might consider setting up a StarletteScope datastructure that inherits from TypedDict or one of the asgiref Scope types to handle this.

Related

• ♻️ Replace _types module by asgiref.typing uvicorn#1044

[br3ndonland]

CI checks are passing, and the PR is now ready for review. Comments and suggestions welcome.

[br3ndonland]

asgiref types added to tests

[tomchristie]

That's a well put together bit of work. Very neatly done. 😌

Having said that, I'm not sure about it yet.
To me it seems more obviously readable to me the current way around, and doesn't have the extra indirection.

Can someone describe what we're gaining by switching to asgiref.typing?

[br3ndonland]

Thanks @tomchristie! 😄 It would be great to get some input from the other maintainers as well, as they're the ones who will be dealing with the types most often.

From my perspective, the use of asgiref.typing here is admittedly a bit of a mixed bag, but I think it's beneficial overall. The main benefit of asgiref.typing is not necessarily the names of the types, but the fields they contain.

Starlette's types are broad:

starlette/starlette/types.py

Lines 3 to 4 in 832bc85

	Scope = typing.MutableMapping[str, typing.Any]
	Message = typing.MutableMapping[str, typing.Any]

In other words, Scope and Message are mappings, with unspecified keys, and values of Any type. It's not much different from typing those fields as just dict. It's left up to the developer to know which keys need to be in each dict, and type checking doesn't provide guarantees that they are passing data through the application correctly.

For example, when instantiating a WebSocketEndpoint:

starlette/starlette/endpoints.py

Lines 51 to 59 in 832bc85

	class WebSocketEndpoint:
	encoding: typing.Optional[str] = None # May be "text", "bytes", or "json".
	def __init__(self, scope: Scope, receive: Receive, send: Send) -> None:
	assert scope["type"] == "websocket"
	self.scope = scope
	self.receive = receive
	self.send = send

We know scope["type"] should be "websocket", but that's about it.

asgiref.typing datastructures are more specific. When using asgiref.typing.WebSocketScope, we can see which keys should be in the dict, which types the values should be, and even the literal value needed for scope["type"].

On the other hand, the greater specificity means type checking is more strict, hence the type narrowing and type: ignore comments added. And Starlette adds many fields to the scope, so as I suggested, I think we would still need some custom types here. It could be more effort to maintain, so I would understand if you didn't want to take on the additional maintenance burden.

Either way, I enjoyed a chance to work broadly across the Starlette code, and I would be happy to help again in the future.

[Kludex]

It looks like we need to discuss this again, but I'm pretty much convinced to vendor asgiref.typing on uvicorn, and I think Starlette should follow the same path. But the review here would take some time.

Let's clean up the PRs we have around, and come back here after 0.19.1, and 0.20.0 are released. Check the milestone.

[br3ndonland]

It looks like we need to discuss this again, but I'm pretty much convinced to vendor asgiref.typing on uvicorn, and I think Starlette should follow the same path. But the review here would take some time.

Let's clean up the PRs we have around, and come back here after 0.19.1, and 0.20.0 are released. Check the milestone.

Sounds good. I'll close this off for now.

Vendoring asgiref.typing would be reasonable. It might help resolve some of the compatibility issues, and we could also extend or modify the types for Starlette's scope extensions and other things (#1511). Hypercorn basically does that.

We should make sure the other maintainers are on board though (encode/uvicorn#1209, encode/uvicorn#1305 (comment)). I think there's still more discussion needed.

[Kludex]

If you don't mind, I'd like to keep this open. 🙇

There's a lot of work that was done here, and in case there's a positive feedback from the others, the only work here would be to change the imports, and vendorizing asgiref.typing - assuming no comments on the review.

[Kludex]

I've been thinking about this...

I don't think that adding asgiref as a dependency would be a good thing for Starlette. I guess I'd go further than that, I'm not sure if there's a real benefit on adding a more ASGI type annotation. In any case, I think you were right @br3ndonland, we should discuss about it.

I'm going to close this, and turn the issue this PR refers to into a discussion. Let's see what others have to say - I'd also like to read your opinion.