-
-
Notifications
You must be signed in to change notification settings - Fork 541
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
fix(maker-wix): version with pre-release tag breaks app start #3855
fix(maker-wix): version with pre-release tag breaks app start #3855
Conversation
`electron-wix-msi` expects a valid semantic version as the `version` option. It then uses the original semantic version as an app version and transformed to windows compatible version as the MSI version separately. Providing windows compatible but invalid semantic version breaks app running via StubExecutable. Signed-off-by: Grigorii K. Shartsev <[email protected]>
034a401
to
1001ded
Compare
packages/maker/wix/src/MakerWix.ts
Outdated
const { version } = packageJSON; | ||
if (version.includes('-') || version.includes('+')) { |
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.
Needs import semver from 'semver'
above but:
const { version } = packageJSON; | |
if (version.includes('-') || version.includes('+')) { | |
const { version } = packageJSON; | |
const parsed = semver.prerelease(version); | |
if (Array.isArray(parsed) && parsed.length > 0) { |
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'm fine with using semver
package to check the version components (I thought about it but didn't want to add it to have one more dependency just to show one log message).
But I intentionally added a new check for +
because prerelease is not the only part of semver that will be changed. There is also build
part.
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.
Ah, thanks! Didn't realize there was that distinction between +
and -
so I had to re-read the spec.
Since we're already including semver
in many places across the monorepo (including @electron-forge/core
, I think adding the dep to maker-wix
is kind of "free" in a sense!
Signed-off-by: Grigorii K. Shartsev <[email protected]>
I was just about to push squashed commit ༼ つ ◕_◕ ༽つ Thanks for a quick review! |
Resolves
Details
@electron-forge/maker-wix
replaces the version (semantic version) with the windows compatible, making it an invalid semantic version, e.g.1.2.3-beta
->1.2.3.0
electron-wix-msi
expects a semanticversion
as an input option and then separates it into 2 versions:semanticVersion
- used as an actual app version, e.g.1.2.3-beta
windowsCompliantVersion
- used as an.msi
version that follows the Windows version format and doesn't support semver, e.g.1.2.3.0
StubExecutable
expects the app to be inapp-{semver}
folder and search to the latest semantic version@electron-forge/maker-wix
passes an invalid semantic version as the versionStubExecutable
cannot run the appAs a result, building with a version with a pre-release tag or build metadata results in an unstartable installation.
Notes
I kept the note. But I think it can be moved to the documentation instead.