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

Fix locationType deprecations. #2721

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

Conversation

calcaide
Copy link
Collaborator

Description

Within Desktop Client, we never deprecated auto location, more info about this deprecation here. Therefore, when running the DC in the browser we were seeing 2 deprecations messages:

  1. Using auto as value has been deprecated. So only hash or history are accepted.
  2. Using detection code at locationType setting.

To solve 1, we decide to change auto to use history. As per documentation, auto will uses history if supported by the browsers and fallback to hash if not. Since we are pretty sure the browser used by electron supports history, is safe to directly uses history.

To solve 2, we decide to set a const with the value before we are setting the environment config, therefore at setting time the variable has already a value.

Screenshots:

Before, showing deprecation messages:
Screenshot 2025-03-11 at 1 56 19 PM

After, showing no deprecation messages
Screenshot 2025-03-11 at 1 57 52 PM

@calcaide calcaide self-assigned this Mar 11, 2025
@calcaide calcaide requested a review from a team as a code owner March 11, 2025 21:07
Copy link

vercel bot commented Mar 11, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
boundary-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 11, 2025 11:00pm
boundary-ui-desktop ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 11, 2025 11:00pm

Copy link
Collaborator

@lisbet-alvarez lisbet-alvarez left a comment

Choose a reason for hiding this comment

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

Thank you for cleaning this up, great catch!!
this doesn't really affect anything but we should probably still clean-up this 'auto' usage too

lisbet-alvarez
lisbet-alvarez previously approved these changes Mar 11, 2025
Copy link
Collaborator

@lisbet-alvarez lisbet-alvarez left a comment

Choose a reason for hiding this comment

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

Thank u for fixing this!

@calcaide
Copy link
Collaborator Author

Thank you for cleaning this up, great catch!! this doesn't really affect anything but we should probably still clean-up this 'auto' usage too

Great catch!!

Not that I am looking to put effort in rose, since we know eventually we want to deprecate it, but this one I agree with you takes no time to fix it, so I pushed the change! thanks for the catch @lisbet-alvarez

Copy link
Collaborator

@lisbet-alvarez lisbet-alvarez left a comment

Choose a reason for hiding this comment

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

Looks great!!

Copy link
Collaborator

@DhariniJeeva DhariniJeeva left a comment

Choose a reason for hiding this comment

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

good catch! thanks for the cleaning this up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants