Apollon77 / daikin-controller-cloud

Connect and Control Daikin Cloud devices
MIT License
98 stars 25 forks source link

Alpha1 Preparations #140

Closed Apollon77 closed 3 months ago

Apollon77 commented 3 months ago

I decided to not return the Daikin URL as "Auth start URL" but the one from the local server that will also handle the response. Because of the self signed process it is better to let the user accept the certificate directly before he does it and not failing at the end because wondering :-) So I added that the redirect happens there

Apollon77 commented 3 months ago

@jacoscaz @JeroenVdb WDYT?

JeroenVdb commented 3 months ago

Looks good!

JeroenVdb commented 3 months ago

@Apollon77 what is actually the difference with oidc_callback_server_baseurl and oidc_callback_server_addr? Should the ip/domain and ports not be the same for both?

Apollon77 commented 3 months ago

@JeroenVdb The oidc_callback_server_addr is the "local binding address" for the server and so kind of also the network interface to use or such. The oidc_callback_server_baseurl is the complete "https url how this server is reachable".

So e.g. also for docker users you migt bind to 0.0.0.0 which is in the docker container. Then you need to forward the port and your docker then has an external IP/name (if not host mode is used). so the oidc_callback_server_baseurl always need to be https://ip-or-name:port where the server is "reachable from your computer where the browser is used for the authentication".

We could rename the oidc_callback_server_addr to include "bind" to make it more clear and we could have oidc_callback_server_baseurl to include" external" or such?

We could also allow oidc_callback_server_baseurl to bne empty when we start the server and we automatically create it as https://${oidc_callback_server_addr}:${oidc_callback_server_port} for these cases that would be possible too

Opinions?

jacoscaz commented 3 months ago

General comments before I look at the code:

I decided to not return the Daikin URL as "Auth start URL" but the one from the local server that will also handle the response. Because of the self signed process it is better to let the user accept the certificate directly before he does it and not failing at the end because wondering :-)

This is some excellent user-oriented thinking! +1!

We could rename the oidc_callback_server_addr to include "bind" to make it more clear and we could have oidc_callback_server_baseurl to include" external" or such?

+1 to renaming to oidc_callback_server_bind_addr.

We could also allow oidc_callback_server_baseurl to bne empty when we start the server and we automatically create it as https://${oidc_callback_server_addr}:${oidc_callback_server_port} for these cases that would be possible too

+1 to defaulting to the concatenation if oidc_callback_server_baseurl is null or undefined.

jacoscaz commented 3 months ago

Also, whilst this change wasn't introduced in this PR, I think having the prepare script in package.json run through the build goes against normal assumptions (npm install is not expected to trigger a build). prepublish would be a better place.

Apollon77 commented 3 months ago

@jacoscaz One other question: Would you be ok if I change the naming convention for the config object to camel case as "more natural" in Nodejs /JS environments? Would be ideal place to do that now directly before breaking later

jacoscaz commented 3 months ago

@Apollon77 absolutely, go ahead. I tend to use snake_case for variables in all of my (recent) projects but I'm aware that it goes against the current.

Apollon77 commented 3 months ago

I will adjust this later and release a first alpha likely tonigth

Apollon77 commented 3 months ago

Ok pushed the comments and some more stuff ... @jacoscaz have a look if you like ... now camel cased it and using real JS "private" with #name

Apollon77 commented 3 months ago

Ok, released as @next on npm ... or ... 2.0.0-alpha.1

https://www.npmjs.com/package/daikin-controller-cloud?activeTab=versions

Apollon77 commented 3 months ago

I have found two issues with busing it and will tweak something tomorrow morning first thing and publish alpha 2

jacoscaz commented 3 months ago

Tested alpha.2, everything looks good!

Apollon77 commented 3 months ago

@jacoscaz alpha 3 is also coming ... will add a method on the main class to update all devices and adds update event to the device class to save requests with these hard rate limits here. If you like contact me in DIscord (Apollon77) and we can have more direct communication there :-)