-
-
Notifications
You must be signed in to change notification settings - Fork 7
change v2 to 2
#41
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
base: main
Are you sure you want to change the base?
change v2 to 2
#41
Conversation
resources/electron/package.json
Outdated
| { | ||
| "name": "nativephp", | ||
| "version": "v2", | ||
| "version": "2", |
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 remember discussing this in another PR. Is it worth making this 2.0.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.
You remember correctly! My personal opinion is no. It's a superficial number. You'll see it when running the serve command for example.
The package.json file is then patched when you build the app, so this version will only be in this file for a brief moment. Considering that the only use I can think of this value is for displaying that vanity string in the cli.
I don't think having to update that manually every release (to keep in sync with minor & patch releases) will add anything
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 cool either way. Just thought it remains consistent with usual versioning standards we use with no expectation of manual update. :)
Approved from my side.
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.
Will 2 cut it? The error seems to be: App version is not a valid semver version
I haven't tested to see whether 2 will succeed
I'm also intrigued by your comments on #39 as it seems to suggest that something else is afoot
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 also intrigued by your comments on #39 as it seems to suggest that something else is afoot
My theory is while the package.json is patched, the lock file is not. We do a npm ci which does not update the lock file, so the v2 lingers around.
I haven't tested to see whether 2 will succeed
The squigglies are gone & I havent seen the error pop up in either mac or windows (yet!)
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.
Just thought it remains consistent with usual versioning standards we use with no expectation of manual update.
We could ofc set it to 2.0.0 and not keep it in sync with the actual version like Pete suggests here. I'm fine with that too.
The only annoyance is my OCD when i see the versions mismatch in the terminal - but i'll live 😆
|
Changed the version to I'll try not to look at it 🙈 |
Happy to poke you in the eyes if it helps <3 :D Thanks, @gwleuverink - Great work. |
The electron package version previously changed to
v2so the cli displayed a pretty string on first run (before package.json is patched) seems to be causing issues on windowsRelated #24
Fixes #39