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: check grid intensity on middleware #26

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

arkgor
Copy link
Collaborator

@arkgor arkgor commented Oct 27, 2022

Problem

IMO checking grid intensity on client side has couple of potential weak points:

  1. It's slow
  2. We have to ask users to grant access to geolocation on their devices

Solution

We can use Next.js middleware (geolocation works well on Vercel for example) to read where the request comes form and perform all the gridIntensity calculations on server-side ... and only if we are not able to do it on server-side, then do it on the browser.

@arkgor arkgor requested a review from sampittko October 27, 2022 19:15
@vercel
Copy link

vercel bot commented Oct 27, 2022

@arkgor is attempting to deploy a commit to the Sustainable UI Team on Vercel.

To accomplish this, @arkgor needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

Copy link
Member

@sampittko sampittko left a comment

Choose a reason for hiding this comment

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

@arkgor like the idea as an exclusive feature for NextJS SUI package. I went with browser API as first approach as it is applicable independent of hosting platform / framework thats being used.

I think we could provide config option to disable browser native geolocation method and give developer freedom to provide location data from source of ther choice - wdyt? We can even expose calculateCarbonIntensity function to let developer choose where it should happen - in serverless function, edge function or on the client.

@sampittko
Copy link
Member

sampittko commented Oct 27, 2022

@arkgor like the idea as an exclusive feature for NextJS SUI package. I went with browser API as first approach as it is applicable independent of hosting platform / framework thats being used.

I think we could provide config option to disable browser native geolocation method and give developer freedom to provide location data from source of ther choice - wdyt? We can even expose calculateCarbonIntensity function to let developer choose where it should happen - in serverless function, edge function or on the client.

Saying that, I think we can make our approach more composable, providing developer possibility to choose where these two actions happen:

  • location data retrieval (browser, middleware, 3rd-party)
  • grid carbon intensity retrieval (browser, serverless function, edge handler)

IMO default should be browser native and we can provide extensions for specific setup i.e. Vercel + NextJS with middleware that would use the library customization options. Would love to discuss more about this.

@sampittko
Copy link
Member

How I think about this package currently is react implementation that can be hooked into raw React app, Gatsby app, NextJS app or any other React-based app. Framework-related improvements should be allowed by exposing customization options in general React library or do you think of any other approach?

@arkgor
Copy link
Collaborator Author

arkgor commented Oct 28, 2022

@sampittko ahh ok, after our call I noted that we go with next.js first and use everything that next.js offers to build the most wow/effective solution first, I had to misunderstood... 🙈

Then yes, for the just react implementation, it does not make sense.

@sampittko
Copy link
Member

@arkgor but it definitely makes sense to provide it for NextJS implementation so if you would like, please go forward with updating the implementation - we can showcase this variant as the more effective one which would be beneficial for the final presentation

@sampittko
Copy link
Member

@arkgor but it definitely makes sense to provide it for NextJS implementation so if you would like, please go forward with updating the implementation - we can showcase this variant as the more effective one which would be beneficial for the final presentation

I see value in providing option in library API to source location from different place than browser API. If you do see it too, dont hesitate to go forward!

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