-
Notifications
You must be signed in to change notification settings - Fork 22
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
defaults: Use hostname for checking development build, instead of NODE_ENV #632
defaults: Use hostname for checking development build, instead of NODE_ENV #632
Conversation
src/assets/defaults.ts
Outdated
@@ -16,7 +16,7 @@ const defaultMiniWidgetManagerVars = { | |||
configMenuOpen: false, | |||
} | |||
|
|||
export const defaultGlobalAddress = process.env.NODE_ENV === 'development' ? 'blueos.local' : window.location.hostname | |||
export const defaultGlobalAddress = window.location.hostname === 'localhost' ? 'blueos.local' : window.location.hostname |
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.
export const defaultGlobalAddress = window.location.hostname === 'localhost' ? 'blueos.local' : window.location.hostname | |
export const defaultGlobalAddress = process.env.NODE_ENV === 'development' || window.location.hostname ==== 'localhost' | |
? 'blueos.local' : window.location.hostname |
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.
This does not solve the problem as the extension is for some reason being published in development mode.
Putting some more thoughts on it, I think we should not do any check at all and always use window.location.hostname
. This is because, when using for development, the setups change a lot. For me, for example, blueos.local
does not work as my Pi is usually connected via Wi-Fi (so needs to be blueos-wifi.local
. For those developing with local backends, it also does not solve, as they will need localhost
as their global address.
In the end, I think always using the hostname is our biggest chance of getting it right, and it also does not introduce extra points of failure. Will change for that.
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.
This does not solve the problem as the extension is for some reason being published in development mode.
Important to notice: I did try to fix it to publish in production mode, but I couldn't find anything wrong. As this issue is a critical one for the user's first experience with Cockpit, I went with this solution that is simple and solves the problem without need for extensive debug.
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.
I think that changing to location.host directly probably will fix the issues related. (blueos.local, blueos-wifi.local, direct-ip..).
Even for near future extensions/widgets that needs to access other ports, by default they coudl try the same address where Cockpit is being hosted.
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.
You mean host
, which includes the port? My concern with that is for applications like BlueOS, which are behind a reverse proxy (Nginx).
4da556e
to
38776a5
Compare
Fix #525