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

Publish version 17 to npm #1701

Closed
3 tasks done
rotu opened this issue Apr 1, 2024 · 8 comments
Closed
3 tasks done

Publish version 17 to npm #1701

rotu opened this issue Apr 1, 2024 · 8 comments
Labels
enhancement Feature request

Comments

@rotu
Copy link

rotu commented Apr 1, 2024

Preflight Checklist

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • I have searched the issue tracker for a feature request that matches the one I want to file, without success.

Problem Description

The electron-packager package is marked as deprecated, with the following message:

Please use @electron/packager moving forward. There is no API change, just a package name change

Unfortunately, you can't just change the package name since there is no overlap in released versions. also involves a major version bump. In my case, that version bump also came with a breaking regression #1700.

Proposed Solution

It would be nice to retroactively release @electron/packager 17 to npm so one can merely replace the name of electron-packager in package.json.

Alternatives Considered

An alternate (and more confusinger) way to switch over package names is to add a package alias to the consuming package like:

npm i --save-dev electron-packager@npm:@electron/packager@18.0.0 -E

Additional Information

@rotu rotu added the enhancement Feature request label Apr 1, 2024
@MarshallOfSound
Copy link
Member

This is neither easy nor ideal, I'll take a look at #1700

@MarshallOfSound MarshallOfSound closed this as not planned Won't fix, can't repro, duplicate, stale Apr 1, 2024
@rotu
Copy link
Author

rotu commented Apr 2, 2024

This is neither easy nor ideal

How so?

In particular, why would it not suffice to take v17.1.2, change only the package name to @electron/packager and publish the result to npm?

@MarshallOfSound
Copy link
Member

@rotu For security reasons publishing of npm packages that Electron maintains is strictly controlled and mostly automated, though we have break-glass tools for certain scenarios in this case there is no reason to go outside our standard publishing practices.

@rotu
Copy link
Author

rotu commented Apr 2, 2024

@rotu For security reasons publishing of npm packages that Electron maintains is strictly controlled and mostly automated, though we have break-glass tools for certain scenarios in this case there is no reason to go outside our standard publishing practices.

Okay. So how does this publishing process foreclose on making a 17-series release? (One would think pushing a v17.1.2-0 tag would cause a release, but I don't understand the CI scripts). And how did a hard breaking change get through this process on a patch version?

@MarshallOfSound
Copy link
Member

MarshallOfSound commented Apr 2, 2024

One would think pushing a v17.1.2-0 tag would cause a release

One would be wrong, I'm not going to give you an essay on how Electron's npm publishing system works. You can look up various components of it, e.g. CFA or read up on why the practice of publishing a semanticly versioned package out-of-order is considered bad. But that's all moot as the answer was no, we won't be publishing a v17 of the scoped package name.

And how did a hard breaking change get through this process on a patch version?

It didn't, this change was published as version 18 and therefore is allowed to have breaking changes, in this case this was an unintentional side effect of a build system change (from cjs to compiled esm typescript) which changed the export semantics without folks realizing and as such to ease the migration I've already patched v18 to support the old syntax

@rotu
Copy link
Author

rotu commented Apr 2, 2024

One would be wrong, I'm not going to give you an essay on how Electron's npm publishing system works. You can look up various components of it, e.g. CFA

Thanks for the refs re: CFA! I'll study up!

or read up on why the practice of publishing a semanticly versioned package out-of-order is considered bad. But that's all moot as the answer was no, we won't be publishing a v17 of the scoped package name.

That's exactly backwards. Semantic versioning is specifically designed so that fixes can be added to previous major branches without upsetting the order. Only bumping the most significant (major) number will cause that release to take priority over the current latest release.

And how did a hard breaking change get through this process on a patch version?

It didn't, this change was published as version 18 and therefore is allowed to have breaking changes

Yes, it did. As written up in #1700, the API change happened in 18.1.2 vis-a-vis 18.1.1, NOT the bump from major version 17 to 18. You can see the sizable diff with npm diff --diff=@electron/packager@v18.1.1 --diff=@electron/packager@v18.1.2.

old src/index.js:

module.exports = async function packager (opts) {

new dist/index.js:

Object.defineProperty(exports, "__esModule", { value: true });
exports.serialHooks = exports.packager = exports.allOfficialArchsForPlatformAndVersion = void 0;
const hooks_1 = require("./hooks");
Object.defineProperty(exports, "serialHooks", { enumerable: true, get: function () { return hooks_1.serialHooks; } });
const packager_1 = require("./packager");
Object.defineProperty(exports, "packager", { enumerable: true, get: function () { return packager_1.packager; } });
const targets_1 = require("./targets");
Object.defineProperty(exports, "allOfficialArchsForPlatformAndVersion", { enumerable: true, get: function () { return targets_1.allOfficialArchsForPlatformAndVersion; } });
exports.default = packager_1.packager;
__exportStar(require("./types"), exports);

@rotu
Copy link
Author

rotu commented Apr 2, 2024

Okay, so I think I understand the build system a little better.

  1. semantic-release doesn't actually check that changes are semantic. It only aggregates commit messages and bumps the version according to the level-of-impact described by the commit message.
  2. CFA is merely semantic release where a human must approve part of the CI process.
  3. Releases for this repo are run from the main branch. So to backport a hotfix, you'd have to expand that filter to include maintenance branches (?).
  4. Since the pre-name-change tags are formatted as expected, it should be as simple as creating a maintenance branch after the v17.1.2 tag and pushing a fix commit.

@rotu
Copy link
Author

rotu commented Apr 2, 2024

Additionally, something should probably be done about the broken releases in 18.1.2, 18.1.3, 18.2.0, 18.3.1. I think these are supposed to be deprecated but maybe @MarshallOfSound has opinions on how seriously we take semver 'round these parts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request
Projects
None yet
Development

No branches or pull requests

2 participants