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

feat: Add option for whether to check package.json semver compliance #7639

Closed
wants to merge 3 commits into from

Conversation

james-pre
Copy link

This PR adds the checkPackageVersion config option. When set to false, the version field of package.json will not be checked for semver compliance. When set to true (the default), it will have the current behavior.

@changeset-bot
Copy link

changeset-bot bot commented Jun 27, 2023

🦋 Changeset detected

Latest commit: 8d9f383

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
app-builder-lib Minor
dmg-builder Minor
electron-builder-squirrel-windows Minor
electron-builder Minor
electron-forge-maker-appimage Minor
electron-forge-maker-nsis-web Minor
electron-forge-maker-nsis Minor
electron-forge-maker-snap Minor

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

@netlify
Copy link

netlify bot commented Jun 27, 2023

Deploy Preview for car-park-attendant-cleat-11576 ready!

Name Link
🔨 Latest commit 8d9f383
🔍 Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/649b43e4e3d6610008b294bf
😎 Deploy Preview https://deploy-preview-7639--car-park-attendant-cleat-11576.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@james-pre james-pre changed the title Add option for whether to check package.json semver compliance feat: Add option for whether to check package.json semver compliance Jun 27, 2023
@mmaietta
Copy link
Collaborator

Is this implementing a specific issue? Looking to understand the context as to why we wouldn't want to verify semver compliance.

@james-pre
Copy link
Author

This is related to #7173: For applications not published to a registry, the version field of package.json does not need to be semver. When trying to build when this is the case, it fails regardless of whether extraMetadata.version is set.

@mmaietta
Copy link
Collaborator

@dr-vortex semver validation for Windows was already removed in Oct 2022.
#7174

@mmaietta mmaietta closed this Jul 10, 2023
@james-pre
Copy link
Author

@mmaietta I saw this PR. Semver validation was not removed for the version field of package.json.

@mmaietta
Copy link
Collaborator

I'm always wary of adding a new option to the configuration but IIRC, this PR also broke numerous unit tests. I'll reopen this, but you'll need to take an alternative approach. What is your version format?

@mmaietta mmaietta reopened this Jul 11, 2023
@james-pre
Copy link
Author

@mmaietta I'm using a non-semver for version. Since I depend on the version in other parts of the application, I put it in package.json instead of using config specific to electron-builder.

The tests may need to be updated if they check against the old config interface.

@james-pre
Copy link
Author

Also, I have not made any commits since I renamed the PR so checks will need to be run again.

@mmaietta
Copy link
Collaborator

Hi @dr-vortex, I've rerun all tests and they're still failing due to changes in this PR

@james-pre
Copy link
Author

The semantic versioning enforcer (which checks for PR title prefixes) is failing despite the prefix existing.

@mmaietta
Copy link
Collaborator

Incorrect. Please see Test / test-mac and Test / test-linux test failures

@github-actions
Copy link
Contributor

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Sep 17, 2023
@james-pre
Copy link
Author

Note that this is still broken.
Resolves #7789

@github-actions github-actions bot removed the Stale label Sep 21, 2023
@mmaietta
Copy link
Collaborator

Note, your PR is still broken
#7639 (comment)

@james-pre
Copy link
Author

Note that this is still broken.
Resolves #7789

I was talking about the PR here

@mmaietta
Copy link
Collaborator

The back-and-forth on this PR disregarding your code failing to pass the unit test suite is rather a nuisance. Closing PR and unsubscribing from this. I'll look forward to more productive conversations elsewhere

@mmaietta mmaietta closed this Sep 26, 2023
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.

2 participants