-
Notifications
You must be signed in to change notification settings - Fork 5
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 geolocation weather example #15
base: main
Are you sure you want to change the base?
Conversation
7d5a4c5
to
a2a679b
Compare
a2a679b
to
f6b0c6a
Compare
+ requires api token for weather site + can provide an ip addr to test with SPIN_VARIABLE_TEST_IP_ADDR=<some ip> SPIN_VARIABLE_WAQI_API_TOKEN=<some token> spin up Signed-off-by: Michelle Dhanani <michelle@fermyon.com>
f6b0c6a
to
1a9e1a2
Compare
@@ -0,0 +1,31 @@ | |||
const getClientAddressFromRequest = (req: Request): string | null => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThorstenHans I borrowed this from one of your js samples. It'd be great to figure out how to consolidate this logic to be used across multiple sample apps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, what are you thinking about? Sharing this across multiple apps/examples would either lead to a local or published npm package.
IMO, being repetitive here is okay, for the sake of each sample being holistic.
Or had you something different in mind @michelleN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, npm package was along the lines of what I was thinking but good point on being repetitive for the sake of being holistic. I think we should keep it as is.
3da16f5
to
5be7c13
Compare
6e0899c
to
1114c8e
Compare
addEventListener('fetch', async (event: FetchEvent) => { | ||
let token = Variables.get("waqi_api_token"); | ||
if (!token) { | ||
event.respondWith(new Response("Internal Server Error", {status: 500})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also possibly make sense to leave a console.log here as to why the request failed to make it easier to debug. My apologies for not leaving it with the previous review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated 👍
Co-authored-by: Karthik Ganeshram <karthik.ganeshram@fermyon.com> Signed-off-by: Michelle Dhanani <michelle@fermyon.com>
1114c8e
to
efa736b
Compare
Signed-off-by: Michelle Dhanani <michelle@fermyon.com>
SPIN_VARIABLE_TEST_IP_ADDR= SPIN_VARIABLE_WAQI_API_TOKEN= spin up