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

feat: useGelocation #165

Merged
merged 3 commits into from
Dec 5, 2024
Merged

feat: useGelocation #165

merged 3 commits into from
Dec 5, 2024

Conversation

huntabyte
Copy link
Member

@huntabyte huntabyte commented Dec 5, 2024

Reactive access to the browser's Geolocation API.

Copy link

changeset-bot bot commented Dec 5, 2024

🦋 Changeset detected

Latest commit: 3988221

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
runed Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@huntabyte huntabyte requested a review from TGlide December 5, 2024 01:54

function updatePosition(position: GeolocationPosition) {
locatedAt = position.timestamp;
coords.accuracy = position.coords.accuracy;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this makes sense here or if we use $state.raw for the coords. This feels more fine-grained but open to feedback.

@@ -8,9 +8,9 @@
export class IsSupported {
#current: boolean = $state(false);

constructor(predicate: () => boolean) {
Copy link
Member Author

@huntabyte huntabyte Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was pretty annoying from a DX perspective not going to lie. Since certain things may be "undefined" you end up always needing to wrap it on the consumer side with Boolean rather than us just handling it which is cleaner imo

Copy link
Collaborator

@abdel-17 abdel-17 Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it. Why do you need to check if navigator even exists? When will it not exist?

Besides, to make it a boolean, it would be navigator !== undefined && "geolocation" in navigator, which is more explicit. No weird truthfy/falsy business.

Copy link
Member Author

@huntabyte huntabyte Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to do navigation ! == undefined is the point. I want to know if it exists/is truthy, which is the type for it. It's either Navigator | undefined, and the same applies to all other Web APIs which is the entire purpose of this util. We just add more work on the consumer for no gain.

As far as when it won't exist, any non-browser environment, to which the easy button is to just slap an if (BROWSER).

But the question isn't really "are we in a browser?" but rather "can we safely use navigator?" The browser flag is just an indirect proxy for what we actually want to know, adding an extra point of potential failure without any real benefit.

Checking for navigator directly avoids all these issues - it's self-contained, accurate, and requires no maintenance.

It's also setting up what I'm looking to add in a PR which will provide support for custom window, document, navigator, etc. to support iframes, shadow doms, etc. (#167)

Copy link
Contributor

github-actions bot commented Dec 5, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
runed ✅ Ready (View Log) Visit Preview 3988221

@huntabyte huntabyte requested a review from abdel-17 December 5, 2024 15:27
@huntabyte huntabyte merged commit ba8f8c9 into main Dec 5, 2024
4 checks passed
@huntabyte huntabyte deleted the feat/use-geolocation branch December 5, 2024 20:36
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