NASA-IMPACT / veda-ui

Frontend for the Dashboard Evolution project
Other
25 stars 5 forks source link

[1156] Expose a Veda UI EnvConfigProvider for providing env variables #1253

Closed dzole0311 closed 6 days ago

dzole0311 commented 1 week ago

Related Ticket: https://github.com/NASA-IMPACT/veda-ui/issues/1166

Problem

We need a way for future Next.js instances using veda-ui components to pass their environment variables to the library

The previous PR tried to pass env variables as props to our components. However, this might be difficult to maintain and to scale as new environment variables will be added.

This PR introduces a centralized config place through a new VedauiConfigProvider context to allow devs to configure all environment variables in one place via a config object.

Description of Changes

Veda-UI changes

Next.js (PR here):

Notes & Questions About Changes

{Add additonal notes and outstanding questions here related to changes in this pull request}

Validation / Testing

Test that the correct env variables are applied when using the library in Next.js

netlify[bot] commented 1 week ago

Deploy Preview for veda-ui ready!

Name Link
Latest commit 0b13f551fdf0ad881ec617925d7c5fa69a04bdcc
Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/673b0ff2e62650000820c7f7
Deploy Preview https://deploy-preview-1253--veda-ui.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

sandrahoang686 commented 1 week ago

Thanks for working through this approach. Prop drilling is pretty straight forward and an easy workaround for now but the amount of surface area it touches is great... Its not ideal with links either right now but we were able to move fast with it. I'm leaning more towards the context provider, more code now to set it up but we wont have to touch as much code in the longer run, we will have to keep this config up to date though with newly added env vars. But I think its still more scalable and ideal than prop drilling is my 2 cents.

hanbyul-here commented 1 week ago

Thanks for showing both approaches @dzole0311 🙇

Yah I agree with @sandrahoang686 . The need for something that can save the configuration-ish (I am saying -ish since I don't think all the configurations can go to providers.) data has been rising, and this might be time to do so.

This is a bit of implementation detail that doesn't have to be tackled now, but mainly to jot down my thoughts -

  1. The other properties that I think worth going to Context is path for each page ex. e&a page needs dataset page route and vice versa - and I think it is confusing for users to pass datasetpath as a prop to e&a page. (and of course, link)
  2. I wonder if it is better to differentiate the values that come from dotenv vs. not -ex. the context assigns the initial value from dotenv without asking the client to hydrate.
dzole0311 commented 1 week ago

@hanbyul-here @sandrahoang686 thanks for the comments!

The other properties that I think worth going to Context is path for each page ex. e&a page needs dataset page route and vice versa - and I think it is confusing for users to pass datasetpath as a prop to e&a page. (and of course, link)

@hanbyul-here Agreed, this would definitely be confusing. But I think we should provide all route-triggering actions as callbacks within the components so that components don't have to handle routing directly (if that's feasible).

(and of course, link)

For the link, did you mean differentiating between the different routers (like react-router vs next.js routing)?

I wonder if it is better to differentiate the values that come from dotenv vs. not -ex. the context assigns the initial value from dotenv without asking the client to hydrate.

Good point! We could place dotenv vars under an env prop and have separate (e.g theming) or other props within the provider’s config object. I can address this in a follow up as we better define what else could sit in the config.

sandrahoang686 commented 1 week ago

But I think we should provide all route-triggering actions as callbacks within the components so that components don't have to handle routing directly (if that's feasible)

that sounds good