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

ci(): upgrade to electron@19 + add e2e tests #158

Closed
wants to merge 1 commit into from

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Dec 2, 2024

  • upgrade to electron@19
    • upgrade nan binding
    • couldn't upgrade to higher versions due to the need for context isolation/bridging that requires a major rewrite
  • add e2e tests
    • playwright not working on linux (probably due to old electron version)
    • standalone test on built app is working

@abebeos
Copy link
Member

abebeos commented Dec 5, 2024

@ShaMan123 , thanks for this. Looks like high-effort to review. Would you be willing to make a separate PR that includes only(!) the minimum changes for an upgrade to electron 19? You should not introduce or alter anything not needed for the upgrade.

@ShaMan123
Copy link
Contributor Author

I can remove the tests. The rest is needed and is the bare minimum.
I am working now on isolating the logic so it can be used as a node module... the code is not in a good condition.
So I am not sure. Maybe it is best to close this PR and try to make it a node module. Then I can bind the electron app with the latest version as a consumer.

@ShaMan123
Copy link
Contributor Author

Moving to electron latest will require this anyways

@abebeos
Copy link
Member

abebeos commented Dec 5, 2024

this

You mean the decoupling to a node_module?

@abebeos
Copy link
Member

abebeos commented Dec 5, 2024

I can remove the tests. The rest is needed and is the bare minimum.

I would prefer to have the tests in a follow-up PR.

@@ -12,32 +12,64 @@ jobs:
build:
strategy:
matrix:
node-version: [14.x, 16.x, 18.x, 20.x]
os: [windows-2022, ubuntu-22.04]
# os: [windows-2022, ubuntu-latest]
Copy link
Member

Choose a reason for hiding this comment

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

avoid the use of "ubuntu-latest", future updates can break the build. Better to pin on the versions you know to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

np

@ShaMan123
Copy link
Contributor Author

this

You mean the decoupling to a node_module?

yes

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Dec 5, 2024

I can remove the tests. The rest is needed and is the bare minimum.

I would prefer to have the tests in a follow-up PR.

What do you think in terms of strategy? Shall we go for the node module decoupling or first we merge this PR since it is less disruptive? The decoupling is based on this head anyways.

@abebeos
Copy link
Member

abebeos commented Dec 5, 2024

What do you think in terms of strategy?

Me dizzy. First day I look again at code after a longer general break. If you feel confident that you can go through with this, then choose the strategy yourself.

Just try to have easy verifiable chunks (PRs). I'll give you access-rights, to fasten things up.

@abebeos
Copy link
Member

abebeos commented Dec 5, 2024

strategy

  • PR update to electron 19
  • PR add more tests
  • PR(s) node_module stuff for further electron updates, keep it in increments, avoid super-PRs.

@abebeos
Copy link
Member

abebeos commented Dec 5, 2024

And a general remark: you need to avoid single-commit PRs. Leav the commits for easier review, you can then later squash down the commit-count, sometimes even to one.

@ShaMan123
Copy link
Contributor Author

Will do. I will close this PR and start a new one.
At some point I suggest introducing prettier and lint for code quality and doing a whitespace PR (very disruptive, non reviewable)

@abebeos
Copy link
Member

abebeos commented Dec 5, 2024

At some point I suggest introducing prettier and lint for code quality

why not.

You should have gotten some kind of invite here on github.

@ShaMan123
Copy link
Contributor Author

Got it, thanks.
I am removing electron-settings - seems to cause issues.
Any problems? Should I persist the settings in some app folder?

@ShaMan123
Copy link
Contributor Author

Or shall I use localStorage? Is that valid with electron? Does it persist after a new version?

@ShaMan123
Copy link
Contributor Author

closing in favor of #159

@ShaMan123 ShaMan123 closed this Dec 5, 2024
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.

2 participants