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

Draft: Moves all prosemirror deps to peerDependencies & devDependencies #3487

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

janthurau
Copy link
Collaborator

@janthurau janthurau commented Dec 4, 2022

@netlify
Copy link

netlify bot commented Dec 4, 2022

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 38fe40c
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/638cc0df3d8f3b0008fdbdd8
😎 Deploy Preview https://deploy-preview-3487--tiptap-embed.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 settings.

@janthurau
Copy link
Collaborator Author

@zanemcca Don't have too much experience with peerDependencies, but I believe this should do the job?

Copy link
Contributor

@bdbch bdbch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bdbch bdbch merged commit 30e799c into ueberdosis:main Dec 5, 2022
Copy link

@zanemcca zanemcca left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing this @janthurau! Aside from that one case I noticed I think what you have hear is a fantastic baseline.

I think the next step will be an important one as well.

When you guys upgrade your prosemirror packages in devDependencies it would be ideal if the version ranges in the peerDependencies were extended to include the new package versions instead of moved forward to match the new devDependencies. If you were to move the peerDepenencies forward in lock-step with the devDependencies then it will force us to use that specific package version even though it may not really be necessary and it could cause trouble with our old editor which was built for an older version.

Obviously if you guys run into breaking changes at specific version of whichever package you can and should remove outdated package versions from the peerDependencies but that should be a fairly rare event. I will leave it to you guys to decide how best to mange when to drop support for older packages but the wider the range you can support the easier it is for downstream developers.

Comment on lines +39 to +41
"@tiptap/core": "^2.0.0-beta.193",
"prosemirror-state": "^1.4.1",
"prosemirror-view": "^1.28.2",
"tippy.js": "^6.3.7"
"prosemirror-view": "^1.28.2"
Copy link

Choose a reason for hiding this comment

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

I anticipate that these three remaining tiptap & prosemirror dependencies will still cause trouble. I think if you moved them to devDependencies then things should work more smoothly.

@thet
Copy link

thet commented Dec 9, 2022

I'm not sure what that change should actually solve.
I'm directly depending on individual tiptap packages. Now I had to add all those prosemirror dependencies to my package dependencies.
Not a huge deal though, but also quite some work to get the build running again.

@zanemcca
Copy link

zanemcca commented Dec 9, 2022

@thet The primary benefit is the ability deduplicate prosemirror packages which is absolutely essential for prosemirror to function properly.

The secondary benefit is it loosens up the available ranges of prosemirror packages that can be used which in turn makes it far easier to upgrade packages at our leisure instead of being forced to upgrade everything to a specific version.

The way NPM manages installations seems to exacerbate these issues in my experience. For instance we cannot remove our package-lock.json right now as it would induce the next npm install to pull in duplicate package versions of @tiptap/core and the overrides option for NPM does not work with workspaces. This means our whole build system is running on a delicate balance until we upgrade all of our prosemirror, tiptap and yjs packages to the latest release. That also includes forcing our old and still actively used editor to use newer than designed for prosemirror packages.

@bdbch
Copy link
Contributor

bdbch commented Dec 10, 2022

I'm not sure what that change should actually solve. I'm directly depending on individual tiptap packages. Now I had to add all those prosemirror dependencies to my package dependencies. Not a huge deal though, but also quite some work to get the build running again.

@thet as @zanemcca explained we had a lot of issues with Prosemirror version clashes because of different dependency versions being imported. Prosemirror has a few global id/key stores for plugins and Selector classes that if multiple prosemirror versions are included can break an app.

In the past we tried to fight this by setting fixed prosemirror versions in Tiptap but that still didn't help out enough to get rid of those errors. Now if a Tiptap user would also import a separate prosemirror package on his/her own, it could also reintroduce this problem because the versions included in Tiptap could clash with the ones the user installed independently.

We're currently working on a solution that would trim down this effort to something like npm install @tiptap/core @tiptap/prosemirror if people don't want to handle prosemirror installs themselves.

Related:
#316
https://discuss.prosemirror.net/t/rangeerror-adding-different-instances-of-a-keyed-plugin-plugin/4242/5

@jmamakeesic
Copy link

@bdbch Please see my comment that references a related installation error.

@bdbch
Copy link
Contributor

bdbch commented Jan 25, 2023

Hey @jmamakeesic - if not already done could you open an issue for that so we can track it better?

@jmamakeesic
Copy link

jmamakeesic commented Jan 27, 2023

@bdbch issue: #3664 PR: #3665

andrewlu0 pushed a commit to trybaseplate/tiptap that referenced this pull request Oct 20, 2023
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.

5 participants