Skip to content

Conversation

@benhuangbmj
Copy link

What

Encapsulate the API base in the environment variables.

@benhuangbmj benhuangbmj requested a review from TaylorOne April 27, 2025 02:52
Copy link
Collaborator

@TaylorOne TaylorOne left a comment

Choose a reason for hiding this comment

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

Can I suggest we use this format instead:

${import.meta.env.VITE_API_BASE}/login?useCookies=true

I leave this decision up to you, but using a template literal is

  1. easier to read, showing clearly how the link is constructed
  2. easier to extend
  3. avoids potential issues with new URL

Even better yet:

const apiUrl = import.meta.env.VITE_API_BASE;

{apiUrl}/login?useCookies=true

@benhuangbmj
Copy link
Author

benhuangbmj commented Apr 27, 2025

Can I suggest we use this format instead:

${import.meta.env.VITE_API_BASE}/login?useCookies=true

I leave this decision up to you, but using a template literal is

  1. easier to read, showing clearly how the link is constructed
  2. easier to extend
  3. avoids potential issues with new URL

Even better yet:

const apiUrl = import.meta.env.VITE_API_BASE;

{apiUrl}/login?useCookies=true

Do you have a particular concern of using the URL constructor? In fact, the reason that I opted for URL is to avoid potential issues with template literals. For example, if I use the URL constructor, then either the VITE_BASE_API being http://localhost/ or http://localhost would work, while the former would not work with the template literal `${import.meta.env.VITE_API_BASE}/login?useCookies=true` . That's my thought.

@TaylorOne
Copy link
Collaborator

Sounds good. We'll stick with the URL constructor. I haven't got it to work yet but hopefully soon.

@TaylorOne
Copy link
Collaborator

The change has been made successfully throughout the site.

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.

3 participants