Closed xelahalo closed 8 months ago
Then a comment about extensibility: we probably don't want a browser "enum". You'd instantly stop supporting the ones you omit, or haven't been released yet. Instead, I'd probably be more open-ended and make it a string (at least for data capture), ideally the one directly reported by the browser, i.e., the one that maps to the HTTP header information. Then instead, if you want to map those values to something more statically typed, you can add a transformation to an enum (which stores the origin value), which has an "OTHER" option as well.
I decided not to include any static typing and just go with strings for now.
I also decided to rename WebBrowser
to Website
as it made more sense to me; it felt weird to register a browser at a url, instead it makes more sense that a specific website has supported browsers and a URL at which it is hosted.
Thank you for adding this @xelahalo !
To have something which requires no further discussion and can be merged now, I adjusted the PR as follows:
Browser
type. This was too naive an implementation. Instead, as suggested before, it is more important to focus on who sets this, and what this should be set to. The User-Agent
HTTP header is the best candidate here. Anything else just forces clients to do more parsing on their end, while at the same time losing information.unsupportedBrowsers
feature entirely. Doing this properly would require adding a class which parses this header information and derives information, such as which browser it is. Only then does adding this to CARP core provide any real value. But, in addition, I think some additional analysis of real-world use cases which would want to exclude certain browsers based on this information is warranted before adding this as a feature (#468).
I also added a convenience URL classes and moved the things that was in WebTask to a common place. I tried to keep things backwards compatible but someone should check it out. I am also unsure about the schemas.