-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(linux): productName should be used as the default value when executableName is not set per docs (#9068)
#9068
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
Conversation
…utableName is not set. (electron-userland#8766)
🦋 Changeset detectedLatest commit: 72b2996 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
I saw that
|
|
I changed
|
|
This looks correct but I'll need to double check the full logic flow when I can find a chance. (Currently working on some other high-pri issues) |
|
@hbcraft is this resolving a specific github issue? Just wondering if there's an issue we can link this PR to for closing it |
|
As the id #8766 in the title
|
productName should be used as the default value when executableName is not set per docs (#9068)
|
FYI Because I am uploading to snap, this broke my build. My I fixed the problem by setting |
|
Hmmm thanks for calling that out. I am hesitant to consider this a breaking change as it's technically fixing a bug -> prod behavior is different than what the jsdoc's states the behavior should be. Just curious, what did you set the |
|
Here is the change I made. My Indeed, I figured it would be difficult to consider this a breaking change. Reminds me of this XKCD comic: |
* Executable name change "electron-mail => ElectronMail" in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
* Unintentional "electron-mail => ElectronMail" renaming in v5.3.1 got caused by electron-userland/electron-builder#9068.
|
I think this should be considered a breaking change and probably warrant a new major once it makes it into stable. Even if it's just a bugfix, it still breaks existing setups (the xkcd comic shared by TJ is quite on point 😅) Changing the Another example is #9234 I resolved these issues for myself so I don't mind, but just wanted to share my thoughts |
fixes an upcoming regression caused by electron-userland/electron-builder#9068
|
@Vendicated good call out. I think in the future the default approach should be to change the documentation to match the prod behavior to avoid potentially-breaking changes like this one seems to be. I can revert this such that it doesn't make it into "stable"/ |
|
Not sure honestly. Personally I think the old default was more sensible. Isn't product name meant to be basically free text that allows special characters and spaces? Not great for binary names 😅 Maybe it'd make sense to make executableName a required property in a future update? Because name could also contain special characters if you use a scoped name ( |
|
Well I'm just worried of the 236,273+ weekly downloads of devs using Making |
|
I haven't been in this headspace for a bit, so I can't say what I think makes sense to be honest. However, I'll give some thoughts based on the info I do have. It seems this change is likely significant enough to be considered a breaking change since multiple users have run into it already, but I'm not used to this product environment such that I can say if that's actually all that significant. Regarding what to do moving forward, I think it would be reasonable to consider the big picture here across operating systems and deployment targets:
With these considerations in mind, would it make sense to use field x in all situations? Or field y in some others? Or maybe it's all too spread out and convoluted and we don't have time to look for a "perfect" solution, so maybe just pick one, make sure the documentation and the functionality are in line, and put it on a breaking release? For example, you could change the code and the documentation so
Or just keep it simple and go with either Just some thoughts. I don't have time to do all this research and considering, and I appreciate if you don't, either. Do what you think is appropriate ;) Thanks for your thoughtfulness and hard work! So thankful for this package. Keep up the great work! |
|
Thank you all for your input. Will revert this PR change and update the documentation to reflect the previous behavior. |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to master, this PR will be updated. # Releases ## [email protected] ### Minor Changes - [#9333](#9333) [`6a49f85c69a22844729033f023249975f47a28f1`](6a49f85) Thanks [@chroberino](https://github.com/chroberino)! - feat: Allow local nsis-resources via env var `ELECTRON_BUILDER_NSIS_RESOURCES_DIR` - [#9279](#9279) [`b6a34c00c35e42dc279a55d672558ea7badc7fcd`](b6a34c0) Thanks [@iamEvanYT](https://github.com/iamEvanYT)! - feat: support Icon Composer icons for macOS ### Patch Changes - [#9282](#9282) [`836a15c6c70abf8582aaa63603e14f77d5fa3f89`](836a15c) Thanks [@naderhen](https://github.com/naderhen)! - NSIS: Fix non-utilized for Silent Flag in Uninstaller - [#9334](#9334) [`21623e1b037e4509af04e767ca1c1458682b0eba`](21623e1) Thanks [@mmaietta](https://github.com/mmaietta)! - chore: remove "beta" labels from a few features - [#9300](#9300) [`0835fbcac0a0cfb0f34355699812cc85db035ad4`](0835fbc) Thanks [@mmaietta](https://github.com/mmaietta)! - chore: updating pnpm and setting minimumReleaseAge to 1 week - [#9346](#9346) [`d19387174365c85968034149be43d80a39e7335f`](d193871) Thanks [@mmaietta](https://github.com/mmaietta)! - fix: revert PR <#9068> due to breaking change - [#9337](#9337) [`f4d7924a082fbb9113d52782430f82b1f0ffcb52`](f4d7924) Thanks [@mmaietta](https://github.com/mmaietta)! - fix: change default value of disable_wayland depending on electron version (in order to support elevtron >38) - Updated dependencies \[]: - [email protected] - [email protected] ## [email protected] ### Patch Changes - Updated dependencies \[[`836a15c6c70abf8582aaa63603e14f77d5fa3f89`](836a15c), [`21623e1b037e4509af04e767ca1c1458682b0eba`](21623e1), [`6a49f85c69a22844729033f023249975f47a28f1`](6a49f85), [`0835fbcac0a0cfb0f34355699812cc85db035ad4`](0835fbc), [`d19387174365c85968034149be43d80a39e7335f`](d193871), [`b6a34c00c35e42dc279a55d672558ea7badc7fcd`](b6a34c0), [`f4d7924a082fbb9113d52782430f82b1f0ffcb52`](f4d7924)]: - [email protected] ## [email protected] ### Patch Changes - Updated dependencies \[[`836a15c6c70abf8582aaa63603e14f77d5fa3f89`](836a15c), [`21623e1b037e4509af04e767ca1c1458682b0eba`](21623e1), [`6a49f85c69a22844729033f023249975f47a28f1`](6a49f85), [`0835fbcac0a0cfb0f34355699812cc85db035ad4`](0835fbc), [`d19387174365c85968034149be43d80a39e7335f`](d193871), [`b6a34c00c35e42dc279a55d672558ea7badc7fcd`](b6a34c0), [`f4d7924a082fbb9113d52782430f82b1f0ffcb52`](f4d7924)]: - [email protected] - [email protected] ## [email protected] ### Patch Changes - Updated dependencies \[[`836a15c6c70abf8582aaa63603e14f77d5fa3f89`](836a15c), [`21623e1b037e4509af04e767ca1c1458682b0eba`](21623e1), [`6a49f85c69a22844729033f023249975f47a28f1`](6a49f85), [`0835fbcac0a0cfb0f34355699812cc85db035ad4`](0835fbc), [`d19387174365c85968034149be43d80a39e7335f`](d193871), [`b6a34c00c35e42dc279a55d672558ea7badc7fcd`](b6a34c0), [`f4d7924a082fbb9113d52782430f82b1f0ffcb52`](f4d7924)]: - [email protected] ## [email protected] ### Patch Changes - Updated dependencies \[[`836a15c6c70abf8582aaa63603e14f77d5fa3f89`](836a15c), [`21623e1b037e4509af04e767ca1c1458682b0eba`](21623e1), [`6a49f85c69a22844729033f023249975f47a28f1`](6a49f85), [`0835fbcac0a0cfb0f34355699812cc85db035ad4`](0835fbc), [`d19387174365c85968034149be43d80a39e7335f`](d193871), [`b6a34c00c35e42dc279a55d672558ea7badc7fcd`](b6a34c0), [`f4d7924a082fbb9113d52782430f82b1f0ffcb52`](f4d7924)]: - [email protected] ## [email protected] ### Patch Changes - Updated dependencies \[[`836a15c6c70abf8582aaa63603e14f77d5fa3f89`](836a15c), [`21623e1b037e4509af04e767ca1c1458682b0eba`](21623e1), [`6a49f85c69a22844729033f023249975f47a28f1`](6a49f85), [`0835fbcac0a0cfb0f34355699812cc85db035ad4`](0835fbc), [`d19387174365c85968034149be43d80a39e7335f`](d193871), [`b6a34c00c35e42dc279a55d672558ea7badc7fcd`](b6a34c0), [`f4d7924a082fbb9113d52782430f82b1f0ffcb52`](f4d7924)]: - [email protected] ## [email protected] ### Patch Changes - Updated dependencies \[[`836a15c6c70abf8582aaa63603e14f77d5fa3f89`](836a15c), [`21623e1b037e4509af04e767ca1c1458682b0eba`](21623e1), [`6a49f85c69a22844729033f023249975f47a28f1`](6a49f85), [`0835fbcac0a0cfb0f34355699812cc85db035ad4`](0835fbc), [`d19387174365c85968034149be43d80a39e7335f`](d193871), [`b6a34c00c35e42dc279a55d672558ea7badc7fcd`](b6a34c0), [`f4d7924a082fbb9113d52782430f82b1f0ffcb52`](f4d7924)]: - [email protected] ## [email protected] ### Patch Changes - Updated dependencies \[[`836a15c6c70abf8582aaa63603e14f77d5fa3f89`](836a15c), [`21623e1b037e4509af04e767ca1c1458682b0eba`](21623e1), [`6a49f85c69a22844729033f023249975f47a28f1`](6a49f85), [`0835fbcac0a0cfb0f34355699812cc85db035ad4`](0835fbc), [`d19387174365c85968034149be43d80a39e7335f`](d193871), [`b6a34c00c35e42dc279a55d672558ea7badc7fcd`](b6a34c0), [`f4d7924a082fbb9113d52782430f82b1f0ffcb52`](f4d7924)]: - [email protected] ## [email protected] ### Patch Changes - [#9334](#9334) [`21623e1b037e4509af04e767ca1c1458682b0eba`](21623e1) Thanks [@mmaietta](https://github.com/mmaietta)! - chore: remove "beta" labels from a few features Co-authored-by: electron-builder-release-bot[bot] <236325277+electron-builder-release-bot[bot]@users.noreply.github.com>

I have conducted tests in my project and it seems there are no issues.
Fixes: #8766