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

[Bug]: 2.1.x StarterKit imports 2.2.x dependencies #4863

Closed
1 of 2 tasks
Nantris opened this issue Feb 7, 2024 · 5 comments
Closed
1 of 2 tasks

[Bug]: 2.1.x StarterKit imports 2.2.x dependencies #4863

Nantris opened this issue Feb 7, 2024 · 5 comments
Labels
Category: Open Source The issue or pull reuqest is related to the open source packages of Tiptap. Type: Bug The issue or pullrequest is related to a bug

Comments

@Nantris
Copy link
Contributor

Nantris commented Feb 7, 2024

Which packages did you experience the bug in?

starter-kit

What Tiptap version are you using?

2.1.13

What’s the bug you are facing?

Perhaps this is intentional - StarterKit imports 2.2.1 version dependencies, eg "@tiptap/starter-kit#@tiptap/extension-italic@2.2.1"

This seems unexpected.

We don't actually use StarterKit anymore but we have it in our dependencies for the sake of git bisect

What browser are you using?

Chrome

Code example

No response

What did you expect to happen?

2.1.x dependencies would be segregated from 2.2.x

Anything to add? (optional)

I have to lie about "Did you update your dependencies" as we can't due to #4862.

Did you update your dependencies?

  • Yes, I’ve updated my dependencies to use the latest version of all packages.

Are you sponsoring us?

  • Yes, I’m a sponsor. 💖
@Nantris Nantris added Category: Open Source The issue or pull reuqest is related to the open source packages of Tiptap. Type: Bug The issue or pullrequest is related to a bug labels Feb 7, 2024
@github-project-automation github-project-automation bot moved this to Triage open in Tiptap Feb 7, 2024
@Nantris
Copy link
Contributor Author

Nantris commented May 29, 2024

@nperez0111 this issue is still relevant.

The StarterKit should probably import dependencies for its own version, eg 2.2.0 should use the ~2.2.0 range for dependencies. I just found when trying to upgrade that having StarterKit 2.1 installed causes dependencies from 2.2 through 2.4 to be installed. I had gotten everything working with our configuration not realizing this, and now when I remove StarterKit (which we don't use anymore) and run with the true 2.1.x versions only, dozens of tests break. A big bummer, but hopefully avoidable for the future if these are fixed to use tilde instead of caret.

@Nantris
Copy link
Contributor Author

Nantris commented May 29, 2024

I created draft PR #5191 for this but don't have time to properly fill it out at the moment.

@nperez0111
Copy link
Contributor

Eh I'm not certain that I agree with you here @Nantris, the reason that we declare version ranges is about compatibility. The starter-kit is mostly just a conglomeration of other packages, using a very tiny subset of their API's (essentially just a pass through to configure).
Being a declaration of compatibility means that as long as we don't change the API interface needed by it's sub deps then it should be compatible with future versions too. This is the intention behind using a ^ vs. a ~, caret allowing future updates in minors and tilde locking to the minor.

I recommend that if you need for a version to be locked to a specific version, NPM and other package managers have lockfiles to ensure that only a specific version of a package is include. Another option is to not rely on NPM's automatic installation of sub deps (i.e. install the sub deps on your own). As long as they are compatible with the version specified, they will be respected and not duplicated in your project.

As such, I don't think that we will accept this PR, most of the time we want people to move off of older versions. And there is increased maintenance burden to backporting bug fixes & security updates to every minor we have released. I think we prefer only to support major release.

I hope this is made clear to you @Nantris have a good day!

@Nantris
Copy link
Contributor Author

Nantris commented May 30, 2024

Thanks for taking the time to explain the rationale. I understand wanting to move people off of older versions, but that should be as much in the end-developer's hands as possible in my view, since upgrades aren't always as clean as one originally hopes. It's very confusing that a user could end up with a mismash of package versions if they define something like below.

Regarding:

NPM and other package managers have lockfiles to ensure that only a specific version of a package is include

The case below isn't avoided by lockfiles due to the reliance on the carets instead of the tildes.

    "@tiptap/core": "~2.1.0",
    "@tiptap/extension-blockquote": "~2.1.0",
    "@tiptap/extension-code-block-lowlight": "~2.1.0",
    "@tiptap/extension-focus": "~2.1.0",
    "@tiptap/starter-kit": "~2.1.0",

What they actually end up with is some 2.4.x packages due to starter-kit which makes for unexpected and mismatched package versions. Now the user has 2.1.x and 2.4.x packages intermingled, and in our case that did not function the same as 2.1.x packages alone did. So that leaves all users vulnerable to unexpected breaking changes when relying on @tiptap/starter-kit, even if they were to only run yarn install or yarn upgrade, thinking that their version ranges were being obeyed.

We don't rely on it anymore as of yesterday evening, so I have no dog in this race, but I really do think that the starter-kit package should not be installing packages outside of its own minor version range for clarity and consistency reasons. I had to rearrange some code yesterday after we removed starter-kit, but my expectation would have been that removing a dependency we haven't used for months would not affect the functionality of our code.

If the team disagrees with this opinion then feel free to close this issue and the associated PR.

Thanks again for your hard work @nperez0111.

@nperez0111
Copy link
Contributor

I'm sorry, but this is standard practice among packages.

For an example, see react-redux.

You say:

I understand wanting to move people off of older versions, but that should be as much in the end-developer's hands as possible in my view

But this is in your control with a lockfile. A lockfile specifically locks your package version to a specific version that it was installed with.

I understand that this may not be expected for you, but again, if you want specifically 2.1.0 in your app, then you need to specifically include that restriction in your package.json. Otherwise, npm will respect our package configuration to install the latest minors.

yarn upgrade specifically goes and tries to update packages based on these rules and will allow you to take up new versions, if you did not want that then don't run yarn upgrade!

Ultimately, as an app dev you have control in installing things however fine-grained you like. This is standard practice for libraries that ship packages. Locking package versions causes duplication of packages where they were not intended despite being compatible with one another and unnecessarily bloats an app bundle.

@github-project-automation github-project-automation bot moved this from Triage open to Done in Tiptap May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Open Source The issue or pull reuqest is related to the open source packages of Tiptap. Type: Bug The issue or pullrequest is related to a bug
Projects
Status: Done
Development

No branches or pull requests

2 participants