Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Website as a device #465

Merged
merged 1 commit into from
Mar 16, 2024
Merged

Conversation

xelahalo
Copy link
Collaborator

@xelahalo xelahalo commented Mar 7, 2024

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.

@xelahalo xelahalo force-pushed the web-browser-device-configuration branch from 6fa83be to 71ba790 Compare March 8, 2024 15:32
@xelahalo
Copy link
Collaborator Author

xelahalo commented Mar 8, 2024

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.

@xelahalo xelahalo force-pushed the web-browser-device-configuration branch from 71ba790 to 0a54351 Compare March 11, 2024 17:52
@xelahalo xelahalo changed the title Add web browser as a device Add Website as a device Mar 11, 2024
@xelahalo
Copy link
Collaborator Author

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.

@xelahalo xelahalo force-pushed the web-browser-device-configuration branch 2 times, most recently from 57d30cc to 2bbde28 Compare March 12, 2024 13:53
@xelahalo xelahalo requested a review from Whathecode March 12, 2024 14:02
@Whathecode Whathecode mentioned this pull request Mar 13, 2024

Verified

This commit was signed with the committer’s verified signature.
Harry-Chen Shengqi Chen
@Whathecode
Copy link
Member

Whathecode commented Mar 16, 2024

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:

  • I removed the 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.
  • Accordingly, I removed the 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 (List unsupported browsers (user agents) in Website device #468).

@Whathecode Whathecode merged commit b7c726e into develop Mar 16, 2024
3 checks passed
@Whathecode Whathecode deleted the web-browser-device-configuration branch March 16, 2024 21:19
@Whathecode Whathecode mentioned this pull request Mar 18, 2024
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.

None yet

2 participants