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

Tokensaver #14

Merged
merged 4 commits into from
Jul 14, 2021
Merged

Tokensaver #14

merged 4 commits into from
Jul 14, 2021

Conversation

gigatexel
Copy link
Contributor

  • added tokensaver.js for standalone purposes (for instance, the Daikin Cloud currently under development for Home Assistant)
  • added binaries for tokensaver.js

@Apollon77
Copy link
Owner

Hey, thank you, good idea.

Whats about the 2add binaries" .... I can not see that anywhere ...

let tokenSet;

// Fix EventEmitter memory leak warning
process.setMaxListeners(Infinity);
Copy link
Owner

Choose a reason for hiding this comment

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

Did you had such errors? Do you found the source? I never saw them while trying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes had memory leak warnings, most certainly caused by background requests from other apps on the pc - since they all use the proxy (and not only the browser consulting the daikin cloud)
But not sure if it fixes this...

Copy link
Owner

Choose a reason for hiding this comment

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

Clean solution would be to find out where in code it happens (i assume somewhere isn the proxy?) and then increase it there. This also works as hack, yes ;-) Is the error messag giving any additional hin when it happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will look into this further and give feedback here. Errors occur from underlying proxy library I think (not your code)

Copy link
Owner

Choose a reason for hiding this comment

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

I would accept for now, all ok ... for proxy lib we can make a PR or at least an issue if we knowo "where" it happens

@gigatexel
Copy link
Contributor Author

Hey, thank you, good idea.

Whats about the 2add binaries" .... I can not see that anywhere ...

I'm quite new to github, sorry.
How can I add them to the PR? I've stored them here: https://github.com/gigatexel/daikin-controller-cloud/releases

@Apollon77
Copy link
Owner

Apollon77 commented Jul 12, 2021

For the binaries ... how you did build them? What they do? Where is the source? In fact it will not work that way :-)

I think we need to build them dynamically with each release. I do not want to have a manual step here.

@gigatexel
Copy link
Contributor Author

For the binaries ... how you did build them? What they do? Where is the source? In fact it will not work that way :-)

I think we need to build them dynamically with each release. I do not want to have a manual step here.

I installed all dependencies in my local folder (npm install) and then built the bins with pkg example/tokensaver.js
Idea is to have something out of the box so users dont need Node.js and npm. If the upcoming HA-integration based on your code is ready, we will see a lot of users who need their tokens.

@Apollon77
Copy link
Owner

What is "pkg"? never did that. but in fact what we then need is to find the exact commands and add them to the github action as build step and somehow add them also to the release to Github ... I never did that. But it's not option for me to have additional manual steps per release, I have too many other duties and this would cost too much time. So

@Apollon77
Copy link
Owner

PS: you mean? https://www.npmjs.com/package/pkg ??

@gigatexel
Copy link
Contributor Author

Yes, that's the one.

I fully understand you time-concerns. Maybe we should skip the binaries for now?

@Apollon77
Copy link
Owner

PS: Maybe https://github.com/softprops/action-gh-release could do the job ... so adding one more step after npm publish to build the stuff. and thenm one step after creating the release to upload the created files

@Apollon77
Copy link
Owner

Ok, lets try it :-)

1.) add pkg as dev dependency to the package.json that it gets installed, do an npm install and update package-lock.json file too
2.) Add (for convenience) a new script to package.json to create the binaries like name="build-tokensaver", command="pkg tokensaver.js" and test that it works locally with "npm run build-tokensaver"
2.) add https://github.com/Apollon77/daikin-controller-cloud/blob/main/.github/workflows/test-and-release.yml#L106 here something like

      - name: Create Tokensaver binaries
        run: npm run build-tokensaver

3.) Then we check thsat this works :-)
4.) Then (because easier) we add three new action steps (one for each file) like https://github.com/actions/upload-release-asset

But I would say we do this n a second PR, ok? SO I will merge this one here and you add a second?

@gigatexel
Copy link
Contributor Author

I agree.
Made some changes in PR to reflect this

@gigatexel
Copy link
Contributor Author

Ready for merge I think.

@Apollon77 Apollon77 merged commit 7000e2e into Apollon77:main Jul 14, 2021
@Apollon77
Copy link
Owner

Apollon77 commented Jul 14, 2021

Thank you, do you want to give the "auto binaries " a try in a new PR or should I start in a branch? Then we can finalize that together

@gigatexel
Copy link
Contributor Author

So I made the changes you proposed in these files:

  • package.json & package-lock.json using npm install
  • test-and-release.yml
  • README.md

Any suggestions how I can trigger the deploy-step in test-and-release.yml?
I've manually triggered the flow using Actions, but it consistently skips deployment.

As I said before, GitHub newbie :-)

@Apollon77
Copy link
Owner

honestly ... I think we review and merge it and then simply try it :-))

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.

None yet

2 participants