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

fix: allow up to 4 integers in version #350

Merged
merged 7 commits into from
Dec 29, 2022
Merged

fix: allow up to 4 integers in version #350

merged 7 commits into from
Dec 29, 2022

Conversation

viclai
Copy link
Contributor

@viclai viclai commented Dec 20, 2022

Details

As per the following docs, the version specified in the manifest can have up to 4 integers (separated by dots).

https://developer.chrome.com/docs/extensions/mv3/manifest/version/
https://developer.chrome.com/docs/extensions/mv3/manifest/minimum_chrome_version/

The schema currently only allows up to 3, so I updated the validation logic to allow up to 4.

Code of Conduct

@viclai viclai marked this pull request as ready for review December 20, 2022 16:53
@louisgv
Copy link
Contributor

louisgv commented Dec 21, 2022

Thanks for the PR @viclai !

I think the main issue here is to fix the chrome allowed version and not so much the extension version. I would like to keep the manifest version format to adhere to semver, but let's incorporate your fix for the chrome version!

@viclai
Copy link
Contributor Author

viclai commented Dec 22, 2022

@louisgv, I actually opened this pull request in hopes of being able to have the flexibility to specify a fourth integer in the version specified for my Chrome extension (i.e. version), so the minimum Chrome version (i.e. minimum_chrome_version) is actually less important for my use case. Is there a reason why we want to adhere to semantic versioning? Is it possible to stick to the semantic versioning spec for the package.json, but still allow the extension version to be specified using up to 4 integers?

@louisgv
Copy link
Contributor

louisgv commented Dec 22, 2022

@viclai interesting - what's your use case for the 4th number in the version? I suppose we can lessen the restriction on this since it wouldn't change the behavior of our bundler. Just wanted to be aware of the reasoning :D

@viclai
Copy link
Contributor Author

viclai commented Dec 23, 2022

@louisgv, we're thinking about using the 4th number to differentiate versions for builds that we distribute internally (before uploading and publishing to the Chrome web store). Our ideal workflow is somewhat similar to what is mentioned about release candidates in the following link.

https://www.chromium.org/developers/version-numbers/

@louisgv
Copy link
Contributor

louisgv commented Dec 23, 2022

we're thinking about using the 4th number to differentiate versions for builds that we distribute internally (before uploading and publishing to the Chrome web store). Our ideal workflow is somewhat similar to what is mentioned about release candidates in the following link.

Hmm... Actually, I'd recommend sticking to semver if that is the case. The released version should track the code change and the release build. If there was some patch released internally, simply bump the patch number. And then, when it's time to release to the public, bump the patch again (or the minor) - the latest version number would contain the diff from the last to the latest. Each version should represent a new build (internal or external) for your extension anyway, so why not keep it linear.

To differentiate the naming/env of the version, use the version_name field in your manifest override.

The key reason I'm against diverting from semver is that, based on my exp, 3 numbers are more than enough for tracking code-build. Adding another number leads to miscommunication or process bypassing.

For my extension, I bump minor when I release to chrome store. Otherwise for local test and Itero I just have the patch version rolling with my commit/push.

RE: internal extension - check out Itero if you haven't - we integrate with GitHub and automatically bump patch for each push to a staging branch so you can continuously test with your team (esp for stakeholders who don't need to worry about the code). Docs: https://docs.plasmo.com/itero

@viclai
Copy link
Contributor Author

viclai commented Dec 24, 2022

Hmm... Actually, I'd recommend sticking to semver if that is the case. The released version should track the code change and the release build. If there was some patch released internally, simply bump the patch number. And then, when it's time to release to the public, bump the patch again (or the minor) - the latest version number would contain the diff from the last to the latest. Each version should represent a new build (internal or external) for your extension anyway, so why not keep it linear.

To differentiate the naming/env of the version, use the version_name field in your manifest override.

The key reason I'm against diverting from semver is that, based on my exp, 3 numbers are more than enough for tracking code-build. Adding another number leads to miscommunication or process bypassing.

For my extension, I bump minor when I release to chrome store. Otherwise for local test and Itero I just have the patch version rolling with my commit/push.

@louisgv, those are fair points! We haven't necessarily settled on a good convention for how we want to bump the extension version in our continuous workflow, but I was just wondering if we can support this level of flexibility (in case we want to use it later). We actually are currently using the version_name field like you mentioned!

With all that being said, I'm happy to make this fix for just minimum_chrome_version (if that part is still reasonable) while I have this PR open. Otherwise, I can just close this pull request.

RE: internal extension - check out Itero if you haven't - we integrate with GitHub and automatically bump patch for each push to a staging branch so you can continuously test with your team (esp for stakeholders who don't need to worry about the code). Docs: https://docs.plasmo.com/itero

This might be a separate conversation, but I'd be interested to see a demo before we commit/subscribe to Itero. 😄

@louisgv
Copy link
Contributor

louisgv commented Dec 24, 2022

With all that being said, I'm happy to make this fix for just minimum_chrome_version (if that part is still reasonable) while I have this PR open. Otherwise, I can just close this pull request.

Let's have this PR handle the minimum_chrome_version! - I think we can split that version checking code into validateSemver vs validateBrowserVersion

@louisgv louisgv self-assigned this Dec 29, 2022
Copy link
Contributor

@louisgv louisgv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seasoned and ready to be served 👨‍🍳

@louisgv louisgv merged commit cc7702b into PlasmoHQ:main Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants