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

Breaking Change in Minor Version Release (LazyIdentifer > LazyIdentifier) #1684

Closed
omermorad opened this issue Dec 17, 2024 · 12 comments · Fixed by #1685
Closed

Breaking Change in Minor Version Release (LazyIdentifer > LazyIdentifier) #1684

omermorad opened this issue Dec 17, 2024 · 12 comments · Fixed by #1685

Comments

@omermorad
Copy link
Contributor

omermorad commented Dec 17, 2024

Breaking Change in Minor Version Release

In version 6.1.0 of InversifyJS, a breaking change was introduced when the LazyServiceIdentifer typo was fixed (PR #1483). The old export was removed instead of being deprecated, violating Semantic Versioning principles. This breaking change affects downstream libraries relying on LazyServiceIdentifer, including Suites (formerly Automock), a key testing library in the Inversify ecosystem

image

Breaking changes should trigger a major version bump according to Semantic Versioning. Minor versions (6.1.0) should not introduce breaking changes.

The changelog lacks clear documentation about the removal of the old LazyServiceIdentifer. I did not dind any announcement or release note warning maintainers of this removal.

Why This Matters

Projects like Suites ensure a robust testing experience for Inversify users. Suites integrates tightly with Inversify's dependency injection framework, and sudden removals like this disrupt entire testing workflows.

As the maintainer of Suites, I ask you please:

  • Announce on acknowledging the breaking change.
  • Update the release changelog explicitly stating the removal of LazyServiceIdentifer.
  • Use a strategy to avoid introducing breaking changes in minor versions in the future (e.g., deprecation warnings before removal).

Links for Reference

@omermorad omermorad reopened this Dec 17, 2024
@notaphplover
Copy link
Member

I see, @omermorad would you mind going for the PR? We would need to add A LazyServiceIdentifer export which would be a type alias of LazyServiceIdentifier and mark it as deprecated.

@omermorad
Copy link
Contributor Author

@notaphplover this should be released as a patch to '6.0.1'

@notaphplover
Copy link
Member

notaphplover commented Dec 19, 2024

@notaphplover this should be released as a patch to '6.0.1'

@omermorad It's released as a patch at [email protected]. What's your proposal?

@omermorad
Copy link
Contributor Author

@notaphplover I appreciate the quick reply and the release of v6.2.1 🚀. Thank you for your efforts in addressing this!

The breaking change was initially introduced between v6.0.3 and v6.1.0 via PR #1601. As a result, users on versions v6.1.x up to (but not including) v6.2.1 are still affected. 😅

To align with semantic versioning principles, any breaking change introduced in a minor version (such as v6.1.0) should be resolved within the same version range. This means the issue caused by removing the LazyServiceIdentifier export in v6.1.0 should be addressed with a patch release for the v6.1.x series, such as v6.1.1. This approach ensures that users locked to ^6.1.0 (e.g., via package-lock.json or yarn.lock) won’t encounter unexpected breaking changes.

I propose releasing a patch for the v6.1.x series to restore the original LazyServiceIdentifier export. If helpful, I’d be happy to create a PR to implement this fix. 😸

Let me know if further clarification is needed or if there’s anything I can do to assist!

@notaphplover
Copy link
Member

notaphplover commented Dec 20, 2024

Hey @omermorad, you're very welcome 😃.

To align with semantic versioning principles, any breaking change introduced in a minor version (such as v6.1.0) should be resolved within the same version range.

I don't see it that way. After having a look at the docs, I read the following:

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes
MINOR version when you add functionality in a backward compatible manner
PATCH version when you make backward compatible bug fixes

[...]

  1. Patch version Z (x.y.Z | x > 0) MUST be incremented if only backward compatible bug fixes are introduced. A bug fix is defined as an internal change that fixes incorrect behavior.

  2. Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backward compatible functionality is introduced to the public API. It MUST be incremented if any public API functionality is marked as deprecated. It MAY be incremented if substantial new functionality or improvements are introduced within the private code. It MAY include patch level changes. Patch version MUST be reset to 0 when minor version is incremented.

  3. Major version X (X.y.z | X > 0) MUST be incremented if any backward incompatible changes are introduced to the public API. It MAY also include minor and patch level changes. Patch and minor versions MUST be reset to 0 when major version is incremented.

So, in the event of having a 1.0.0 version and releasing changes with both a bugfix and a backward compatible manner, the resulting version would be 1.1.0. According to the docs, we would be including a fix in the 1.1.0 version without releasing a 1.0.1 version. Doesn't this refute your previous statement? I might be missing something though.

Let me know if further clarification is needed or if there’s anything I can do to assist!

Sure, let's clarify this first because I honestly believe releasing the patch in 6.2.1 is more than enough, I understand the point of Semver is being able to update from 6.0.X to 6.2.1 without affording any breaking changes. Could you bring a use case to justify this extra PATCH versions? I would understand if we were in a different major.

@omermorad
Copy link
Contributor Author

@notaphplover I see your point.

But the issue isn’t about including a bug fix in a minor version like 6.2.1—that’s fine. The concern is that a breaking change was introduced in v6.1.0, violating backward compatibility within the ^6.1.0 range. Users locked to ^6.1.0 cannot safely upgrade without jumping to v6.2.1, which is not ideal for projects with dependency constraints.

Releasing a patch for the v6.1.x series (e.g., v6.1.1) to restore the removed LazyServiceIdentifer export ensures users in ^6.1.0 don’t encounter unexpected breaking changes. Does that make sense?

@notaphplover
Copy link
Member

notaphplover commented Dec 20, 2024

Hey @omermorad,

But the issue isn’t about including a bug fix in a minor version like 6.2.1—that’s fine. The concern is that a breaking change was introduced in v6.1.0, violating backward compatibility within the ^6.1.0 range. Users locked to ^6.1.0 cannot safely upgrade without jumping to v6.2.1, which is not ideal for projects with dependency constraints.

[email protected] is in the range of inversify@^6.1.0 as you can see in the semver calculator:

image

If you mean users constraining inversify@~6.1.0, it's such a niche case I don't have plans to cover it. They can safely upgrade to [email protected] whatsoever 😃.

@omermorad
Copy link
Contributor Author

@notaphplover apologies for the confusion—I meant users limited to ~6.1.0, and not to ^6.1.0, I understand your perspective, maybe it is unnecessary to mention this since 6.2.1 addresses the problem for the majority of users :)

Thank you, really appreciate the effort 🙌

@notaphplover
Copy link
Member

You are very welcome @omermorad, I really appreciate your interest in improving the library and providing the best developer experience ❤️. I'm well aware we need to improve in this area, work is going on in the monorepo and I am confident I'll be able to deliver a great major in a couple of months :).

@omermorad
Copy link
Contributor Author

Of course @notaphplover :)

Oh that's nice! If possible, I’d love to hear more about what’s planned. I could even prepare @suites/di.inversify in advance to ensure it’s fully compatible and aligned with the new version you're planning.

Speaking of which, what do you think about adding a dedicated file in the docs to showcase how Suites integrates with Inversify for testing use cases (instead of Automock in wijki)? I think it could be really valuable to the community! I’d be happy to contribute, I can provide some examples and even advanced testing techniques. We could open a separate issue to discuss this further if you’re interested 🚀

@notaphplover
Copy link
Member

Hey @omermorad :)

Oh that's nice! If possible, I’d love to hear more about what’s planned. I could even prepare @suites/di.inversify in advance to ensure it’s fully compatible and aligned with the new version you're planning.

That would be great, yeah. Regarding the next major, I'm working on it, I will create a discussion in the repo when I'm ready to show you guys the proposal. Don't worry, I won't release inversify@7 without a proposal, a gh discussion and some time to do some beta testing with users. The current focus is releasing a nice API which is as useful to users as the current one without exposing internal types only inversify internal modules should be aware of. Some enhancements are on the way, performance of inversify@7 should be way better than any other inversify version thanks to the design I have in mind. Some bugs will be finally resolved thanks to a complete refactor of the core. I'll tell you more once I start the discussion ;).

Speaking of which, what do you think about adding a dedicated file in the docs to showcase how Suites integrates with Inversify for testing use cases (instead of Automock in wijki)? I think it could be really valuable to the community! I’d be happy to contribute, I can provide some examples and even advanced testing techniques. We could open a separate issue to discuss this further if you’re interested 🚀

Docs are going to be implemented via docusaurus in the monorepo. I'm currently the only active maintainer of the library so I'll need time to complete what I have in mind, but feel free to submit an issue or start a discussion in the monorepo so I don't forget about it

@omermorad
Copy link
Contributor Author

@notaphplover Sounds good!
Curious about Inversify 7 - keep me in the loop 😉

I’ll go ahead and submit an issue about showcasing how Suites integrates with Inversify in the monorepo, as you suggested.
Keep up the great work! 🚀

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 a pull request may close this issue.

2 participants