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

2.0.0 #111

Merged
merged 6 commits into from
Sep 7, 2023
Merged

2.0.0 #111

merged 6 commits into from
Sep 7, 2023

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented Sep 7, 2023

This is the release candidate for version 2.0.0.

github-actions and others added 2 commits September 7, 2023 05:33
@legobeat legobeat requested a review from a team as a code owner September 7, 2023 05:42
@legobeat
Copy link
Contributor Author

legobeat commented Sep 7, 2023

CHANGELOG.md Outdated Show resolved Hide resolved
@legobeat legobeat requested a review from Mrtenz September 7, 2023 07:13
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
@legobeat legobeat requested a review from Mrtenz September 7, 2023 07:30
@FrederikBolding
Copy link
Member

While this is technically a breaking change in the type I don't think it's really necessary to do a major bump for it. What do you think?

@Mrtenz
Copy link
Member

Mrtenz commented Sep 7, 2023

While this is technically a breaking change in the type I don't think it's really necessary to do a major bump for it. What do you think?

If it's breaking, technically or not, I think we should do a major bump. 😅

@legobeat
Copy link
Contributor Author

legobeat commented Sep 7, 2023

While this is technically a breaking change in the type I don't think it's really necessary to do a major bump for it. What do you think?

If we consider that this was a type-only change - TypeScript being a developer tool and strictly optional - I would tend to agree that breaking type interfaces shouldn't necessitate a breaking change.

However:

  1. Treating breaking TypeScript interfaces as semver-major is in line with current and recent practice in other repos in the MetaMask org. So it's a bit of a separate discussion.
  2. This does in fact change runtime behavior so it's breaking even if we disregard that IMO

I guess two options where both sound reasonable to me:

  1. revert Validate snap ids #75 -> release 1.3.0 -> re-apply Validate snap ids #75 -> release 2.0.0
  2. go ahead with 2.0.0 in one go as in this pr

@FrederikBolding
Copy link
Member

This does in fact change runtime behavior so it's breaking even if we disregard that IMO

Yeah, you are right. However, for our use we do not currently rely on the runtime behaviour. Which is why I brought it up.

But I am fine with following semver properly here as well. Just wanted to voice the option, because it wouldn't be breaking with our usage.

@Mrtenz Mrtenz merged commit a689dc9 into MetaMask:main Sep 7, 2023
21 checks passed
legobeat added a commit to legobeat/snaps-registry that referenced this pull request Sep 7, 2023
This reverts commit a689dc9.

The release failed and will be rescheduled as-is.
Mrtenz pushed a commit that referenced this pull request Sep 7, 2023
This reverts commit a689dc9.

The release failed and will be rescheduled as-is.
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.

3 participants