Skip to content

Conversation

eerohele
Copy link
Contributor

Because it reflects, the ring.adapter.jetty/proxy-handler function showed up quite prominently on a recording of an app running in production.

This change fixes all reflection warnings in the ring.adapter.jetty namespace.

Would you be willing to entertain a PR that enables reflection warnings throughout the project, either via :global-vars {*warn-on-reflection* true} or (set! *warn-on-reflection* true) at the top of every namespace that does Java interop (perhaps coupled with the :warn-on-reflection clj-kondo linter)?

@weavejester
Copy link
Member

Thanks for the PR - I'm not sure how I missed this!

I would love a PR that enables reflection warnings, but it's more work than it sounds. Ideally we just add *warn-on-reflection* to the global vars and run the tests, but the tests themselves have a lot of reflection, so any warnings on the source are lost in the noise of all the warnings on the tests.

We could set *warn-on-reflection* only on the source namespaces, but I don't think Leiningen has a way of doing that automatically, and I don't like the idea of adding (set! *warn-on-reflection* true) to every namespace either.

So that leaves going through the tests and fixing all the reflection, which seems like the correct solution, but is also a fair bit of work. This is why I elected to test all the namespaces manually before releasing 1.15.2, but clearly I somehow missed the reflection in the Jetty adapter.

@weavejester weavejester merged commit 0ea6f8d into ring-clojure:master Sep 23, 2025
1 check passed
@eerohele
Copy link
Contributor Author

eerohele commented Sep 23, 2025

So that leaves going through the tests and fixing all the reflection, which seems like the correct solution, but is also a fair bit of work.

I can take a look at how much work this would entail.

Thanks for the quick turnaround!

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.

2 participants