Update latest pekko-http to bring in cors #106
Conversation
mdedetrich
force-pushed
the
update-latest-pekko-http-bring-in-cors
branch
6 times, most recently
from
4b4e349
to
0563fb1
Compare
|
|
||
| @ApiMayChange | ||
| object WebHandler { | ||
|
|
||
| /** Default CORS settings to use for grpc-web */ | ||
| val defaultCorsSettings: CorsSettings = CorsSettings.defaultSettings | ||
| val defaultCorsSettings: CorsSettings = CorsSettings(ConfigFactory.load()) |
There was a problem hiding this comment.
@lomigmegard So I am trying to get rid of the now deprecated CorsSettings.defaultSettings, do you think this is fine for now given that we don't want to change the API for the first release? Passing either Config or ActorSystem as a parameter to defaultCorsSettings while changing it to a def would require more significant changes.
I suspect that this API can use improvements in future versions (and it is marked as @ApiMayChange).
There was a problem hiding this comment.
Can this refers to actorsystem's config? or will have another config instance.
There was a problem hiding this comment.
Making this refer to the ActorSystem's config will require more significant code changes, i.e. defaultCorsSettings will have to change to a def and due to the usage of implicit default paramters in https://github.com/mdedetrich/incubator-pekko-grpc/blob/0563fb1672c6ca831423d02925687c8635fd0d03/runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/WebHandler.scala#L63 it will also require rewiring of code.
Or as you said it will need us to create a new actor system instance.
There was a problem hiding this comment.
we want to retain good compatibility with akka-http-cors - so big refactors should wait till a later pekko-http release
There was a problem hiding this comment.
Also to note, CorsSettings(ConfigFactory.load()) is semantically the same as the original CorsSettings.defaultSettings
There was a problem hiding this comment.
@mdedetrich that should be fine yes, given the constraints of not changing the interface of this field.
There was a problem hiding this comment.
Thanks, merging then!
| @@ -32,8 +34,6 @@ import pekko.japi.function.{ Function => JFunction } | |||
| import pekko.stream.Materializer | |||
| import pekko.stream.javadsl.{ Keep, Sink, Source } | |||
| import pekko.util.ConstantFun | |||
| import ch.megard.pekko.http.cors.javadsl.settings.CorsSettings | |||
| import ch.megard.pekko.http.cors.javadsl.CorsDirectives | |||
mdedetrich
force-pushed
the
update-latest-pekko-http-bring-in-cors
branch
from
0563fb1
to
dd70c28
Compare
Also removes the now unnecessary Snapshot OSS repo, removes other workarounds and updates pekko core to 1.0.0-RC3.