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

Upgrade electron-forge to 7.5.0 #2526

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

Conversation

calcaide
Copy link
Collaborator

@calcaide calcaide commented Oct 18, 2024

Description

  • Upgrade electron-forge to latest.
  • Delete lodash patch within electron-app.

The reason behind this changes is electron-forge/packer changed the way it validates semver for windows, therefore, the pattern we used before turns out invalid and returns an error when building the desktop client. More info about the issue within this GH ticket. Code that validates the semVer, within packager.

Due to this change, don't support non-numeric version, we add the function returnSemVerFromReleaseVersion to clean up the provided releaseVersion and bein compliance.

Delete lodash patch for electron-app its not need anymore. The new version of electron-forge resolves lodash 4.17.21, as per snyk scan, does not have security vulnerabilities.

How to Test

  • Perform a pipeline run succesfully.
  • Test pipeline run, using the new branch naming pattern.
  • Unit testing (manually) returnSemVerFromReleaseVersion with next string:
    • 1.2.3-beta.test.node.pty = 1.2.3;
    • 0.2.3-beta.electron.forge = 0.2.3;
    • v2.0.3 = 2.0.3;
    • v0.10.3 = 0.10.3;

@calcaide calcaide self-assigned this Oct 18, 2024
Copy link

vercel bot commented Oct 18, 2024

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 Nov 5, 2024 6:03pm
boundary-ui-desktop ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 5, 2024 6:03pm

…upport for non-numeric appVersion in windows
major = major.replace('v', '');
}

return major.concat('.', minor, '.', parseInt(patch));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we parseInt(patch) to basically remove any text that may have been attached to it? e.g. 1-beta in 3.2.1-beta? If so, perhaps we could add a comment to explain that?

Could this also have just used a template string?

Suggested change
return major.concat('.', minor, '.', parseInt(patch));
return `${major}.${minor}.${parseInt(patch)}`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes.

  • The parseInt for patch was add for that exact reason. Good catch, added a comment.
  • I also move the comment to explain returnSemVerFromReleaseVersion.

Copy link
Collaborator

@cameronperera cameronperera left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for digging into this issue and coming up with a solution for the version.

Copy link
Collaborator

@ZedLi ZedLi left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this!

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.

4 participants