Essentially yes. The modules would describe the public API of common things, like PHP’s PSRs. The current types would include these modules, but it would allow other shards to use the module in order to be compatible with non-stdlib implementations.
As someone coming from the PHP world you’re suggesting we copy, I’m not a fan.
For starters, the example gives me the willies with all the *Interface interfaces. I’m tired of tab-completing Interface. Laravel at least has the decency to call the interface Logger rather than LoggerInterface. Has the added bonus that you don’t have to refactor all your usage when you realize your class should be an interface.
And as you’re mentioning decorators. Drupal is built on Symfony these days, which means Drupal has adopted Symfonys way of doing decorators. With the end result that you’ll find a lot of “how to create a service decorator” blog posts that’ll happily tell you that you’ll need a decorator class that inherits from the decorated class (in order to actually implement the interface without having to stub out each method), but forgets to mention that this means that you’ll now have two objects that implements the interface, and you’ll have to be careful (an accidental $this->method(); instead of $this->originalService->method(); can introduce subtle bugs). forward_missing_to might avoid this, but I think it’s important to consider how one might shoot oneself in the foot.
I’m much more interested in implicit interfaces/structural types. For one, it allows one to provide a duck with the right pitch of quack when the original developer didn’t realize an interface would be the way to go.
The only thing I like about declared interfaces is that they can be more granulated, an class can say “yeah, I’m Renderable and Translatable”. Which you don’t quite get with “yeah, I’m a HTTP::Request”, but if there’s a way to tell the compiler that my custom class is interchangeable with a HTTP::Request, it could be possible it’s also compatible with another type.
Besides, I think monkey patching is getting a bad wrap.
Anyway, just my 2 cents.
This is a bit harder to do in Crystal land as the stdlib can’t just define HTTP::Request as a module when it already exists as a class. Plus for things on the top level, Logger could easily conflict with other types people have defined. Granted that could also happen with LoggerInterface but its less likely, and more accurately denotes its not something you can use its own, but is something that needs an implementation.
delegate would probably work better for a decorator in the instances you do not want to customize specific methods. forward_missing_to wouldn’t actually define a method that satisfies the interface.
Ultimately maybe that would be better, but at the end of that day that requires support from the compiler, figuring out how it should work, and how it should be implemented, etc. Whereas we can easily create some modules with abstract defs and start using them right away. At the end of the day is it really a big deal you might see/have to type *Interface in some cases? IMO the benefits outweigh the cons.
I should also reiterate I don’t think we need to get as granular as PHP/Java does in this regard. HTTP::Request (and possibly related types) are really the only things I can see doing this for. Logger could make sense too, and URI, but those are both in the stdlib already and less likely for custom implementations so prob not really worth it.
1 Like
I don’t see why it should be harder? In PHP you also have to move your implementation out of the way first, and then replace it with an interface. I haven’t played around with “modules as interfaces” in Crystal yet, but as I understand it, any method that takes the class in question as a parameter dousn’t care if the type is actually a class or a module included by the class.So as long as you’re injecting your dependencies, the only place you need to change a bit is the place where the thing is actually instanciated in the first place.
Well, some languages goes with Logger and LoggerImpl which I hate even more, Impl tells me nothing really, I’d rather have Logger, DatabaseLogger and FileLogger.
Thing is, keeping the distinction between interface and implementation is bleeding implementation details into the usage of the interface. When using the Logger, I don’t care if its simply a class with an implicit interface, or a FileLogger that implements the Logger interface. I just want to call the notice method, I really don’t care wether theres “Module” or “Class” at the top of the documentation page.
But you’re setting precedence on “how it’s done”, and if a better way is found later it’ll take major versions to back out of again. If ever.
But the major red flag here is that you’re holding up PSR-7 as an example to follow. We have at least 3 different major HTTP clients, and PSR-7 promises to make them interchangeable. Yet, we still run into packages blindingly requiring guzzlehttp, which locks you into using that or accepting two clients in your code base.
The thing is that PSR-7 (and PSR-18) exists because PHP doesn’t have HTTP::Client and nobody could agree on which client to use. So they cooked up a common interface that all could implement.
But Crystal has HTTP::Client, so crest and halite apparently use it under the hood. The thing is, when you pick an HTTP client, it’s usually for a reason (nicer interface perhaps?), while implementing a common interface tries to erase those differences.
Under the hood both guzzlehttp and Symfony HTTPClient uses PHP curl library per default, so what you’re not as much picking a HTTP client implementation, as you’re picking an abstraction. And on top of that you might have HTTPlug to abstract your abstractions.
Quite frankly, I think it’s a mess.
If OAuth2::AccessToken#authenticate uses HTTP::Client, has been tested with it, and works, why would you want to use another client for it?
1 Like
As a breaking change then yea it’s doable. Was mainly thinking trying to do that w/o causing breakages, which is harder since anyone can reopen HTTP::Request and add stuff to it, which would break. But yea, it probably would have to be a breaking change so doesnt really matter in the end. Would be quite a big one tho.
Yea for sure, if the project starts off that way it’s a bit easier to manage. My thought there was just due to how you can monkeypatch and redefine stuff in Crystal. If the stdlib introduces Logger as a module interface at the top level, that could easily conflict with other things, such as the old Logger type in the stdlib. Since they would conflict.
This assumes that a better way will be figured out/implemented soon. Is it worth waiting for something that possibly never happens versus start using the feature right now and later on if it can be replaced, great? Plus this proposed way isn’t anything special. It would take a fundamental change in the langage to make it no longer work, so it’s entirely likely it would continue to work, even alongside of, some future feature like the implicit interfaces mentioned earlier.
Isn’t this more so people just not correctly adhering to PSR-7 rather than PSR-7s fault itself? Introducing this now would mean it has more time to grow in adoption while the ecosystem is small. Versus trying to introduce it into a lot more mature ecosystem.
The reason is mainly Redesign HTTP::Client to implement missing features · Issue #6011 · crystal-lang/crystal · GitHub, you just can’t use it if you need to follow redirects, use a proxy, use HTTP/2 etc. So it could be nice to have some sort of (optional) abstraction that could allow the user to use whatever they want. If you in your own project don’t want to use it and type everything as HTTP::Client that’s totally fine too.
I’m mainly coming at this from a shard creator POV where I want my lib to be flexible and not limit the user to one client lib that I chose as a hard dependency. Having this ability would be a nice way to support that. I also think by virtue of Crystal having a built in client, if other shard creators don’t really want/need that flexibility, they’re able to just not do it and type everything as HTTP::Client and call it a day. We do not have to deal with the provides: psr/http-client-implementation nonsense since there’ll always be a default type that is also compatible with the interface, even if those methods are not publicly exposed for sake of a cleaner API.
Also I should make it clear HTTP Clients are not my main concern. Having the actual request/response object abstracted out is my main use case at the moment. E.g. I want anyone using Athena Framework’s Request - Athena to be able to pass it around as if it was an HTTP::Client::Request. Since it is 100% compatible with it already.
1 Like
Soo, monkey patch? I know it’s a dirty word and you won’t find it in any Design Patterns book, but it’s one of the things that make Crystal (and Ruby) special.
I understand that view. But we have a client in core, which can be monkey patched if need be. “Oh, but monkey patching might break something” I hear people say, yes, but if you have a shard that breaks because HTTP::Client suddenly follows redirects, it would probably fail if you swapped one HTTP::Client interface for another than it was tested against anyway.
You could monkey patch instead… In the context of an Athena app, is there any reason to distinguish between an Athena request and a non-Athena request? A decorator makes sense in a language where you can’t re-open a class and add missing bits, but we’re blessed with a language where this is not the case.
There’s only so much monkey patching can do. And even then, I don’t want to be responsible for touching code I don’t own. Maybe it’s fine for simple things, but trying to monkey patch in everything in that ticket is a bit much, if it would even be possible. And even if you did, needing to keep your duplicated code up to date with upstream is more than I want to deal with.
I do not think the solution to problems like this is “oh just monkey patch it” when there are totally reasonable, relatively simple ways to that could be implemented to solve the use case in a better way. That not only allows for better code design, but also make testing easier by not having to essentially duplicate all the upstream tests to make sure my patches didn’t break anything.
If both clients properly implement the interface they should, from the POV of the shard, act exactly the same. If they break, it would be more on the implementation than just because there was an interface. Which would be no different than HTTP::Client working when halite doesn’t because of a bug. The fact they both could have the same public API via the interface is irrelevant.
1 Like
You’d rather write your own client from scratch? If your own client is just built upon HTTP::Client you’re still touching the code in a more indirect way.
That’s one of the things I like about Crystal/Ruby. In PHP (and many other languages), there’s a clear distinction between “your” and “my” code. I can read your code, but outside of patching it, there’s a clear line. Crystal is much more… Communal, for lack of a better word. There’s no strong border between yours and mine, I am free to patch that hole I perceive in your code.
[quote=“Blacksmoke16, post:27, topic:4414”]
If both clients properly implement the interface they should, from the POV of the shard, act exactly the same. [/quote]
The same argument goes for monkey patching. Whatever method you use, there’s a chance that someone isn’t quite up to spec, some edge case triggers something in the code using the client, etc. Point being that careful patching isn’t inherently more buggy than interfaces.
This is the key difference IMO. By monkey patching YOU are taking ownership/responsibility for its implementation, whereas if you’re using a client created by someone else that implements a common interface, THEY are responsible, even if their implementation is just them monkey patching HTTP::Client itself.
I as a shard creator do not want to take on that responsibility by monkey patching things just to make it work with my code. I do however, want to give the ability for that to happen by letting someone else implement the interface. That’s all. If you don’t want to implement/use the interface modules in your code, that’s fine. If you want to monkey patch instead, that’s fine. But I just don’t think “oh just monkey patch it” is a strong enough argument to warrant not having a better way to handle this use case. At least until when/if a more robust compiler supported feature is available.
Fair enough. Then make your code work with the existing HTTP::Client? If not by monkey patching, then handling it in the client code. Send patches upstream to make the situation better in the next Crystal.
It sounds to me that the HTTP::Client in core is not the HTTP::Client you want, so you’re suggesting we make it pluggable, so you can get what you want, rather than working with what you’ve got.
Ah, but it’s not just “use it or don’t”. It’ll become “how’s things done” and interfaces will start popping up all over the place. Some shards patch HTTP::Client, some use the interface.