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

refactor: Split out slate dom package #5734

Merged
merged 12 commits into from
Oct 31, 2024

Conversation

bmingles
Copy link
Contributor

@bmingles bmingles commented Oct 3, 2024

Description
The slate-react package contains a fair amount of non-react code. It's a combination of DOM + React specific features. This splits out non-react / DOM functionality into a slate-dom package to make it easier for library authors to incorporate Slate without having to rewrite all of the DOM utils.

Context

  • Split out slate-dom package and moved non-react code into it
  • Refactored slate-react package to use slate-dom for its DOM functionality

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

resolves #5731

Copy link

changeset-bot bot commented Oct 3, 2024

🦋 Changeset detected

Latest commit: 2abc522

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
slate-react Minor
slate-dom Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@bmingles bmingles marked this pull request as ready for review October 3, 2024 14:21
@dylans
Copy link
Collaborator

dylans commented Oct 3, 2024

@bmingles thanks! I'll review this asap.

@bmingles
Copy link
Contributor Author

bmingles commented Oct 6, 2024

@dylans FYI I may have accidentally npm published slate-dom. Didn’t even know I was able to do this. I was trying to target a local npm registry for testing but noticed it is now on npm
https://www.npmjs.com/package/slate-dom

Let me know if I need to do anything to address this.

@dylans
Copy link
Collaborator

dylans commented Oct 6, 2024

@dylans FYI I may have accidentally npm published slate-dom. Didn’t even know I was able to do this. I was trying to target a local npm registry for testing but noticed it is now on npm https://www.npmjs.com/package/slate-dom

Let me know if I need to do anything to address this.

Oops... yes, npm let's you publish to any package name that hasn't already been published. We should switch to @slatejs at some point which would block that from being possible.

We can either choose a new name for the package or deal with any issues when we go to publish it (generally the main issue would be that we cannot publish the same version number iirc). It's been a long time since we've added a new top-level package so I don't remember what we need to do, but I'll look when I get a chance.

@bmingles
Copy link
Contributor Author

bmingles commented Oct 6, 2024

It looks like I can add owners to the slate-dom pacakge:

Based on npm owner ls slate, I've invited the following to be owners of slate-dom:

ianstormtaylor <[email protected]>
thesunny <[email protected]>
timbuckley <[email protected]>
damareyoh <[email protected]>
brentfarese <[email protected]>

I believe I can also unpublish the package within first 72 hours if that is preferred since this PR hasn't been approved yet. I'm happy to do whatever.

@bmingles bmingles mentioned this pull request Oct 7, 2024
8 tasks
@dylans
Copy link
Collaborator

dylans commented Oct 8, 2024

@bmingles assuming the 72 hour window wouldn't impact our ability to publish the package later, I'd say unpublish it. Otherwise sharing it with the owners is fine. Somehow I was never added to the owner list for npm so I'll get that sorted out separately.

Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

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

I'm pretty much fine with these changes. I'm going to leave it open for a few days in case anyone else has feedback.

Beyond that we need to look at our release scripts and probably make some updates so that the new package gets published with each release, etc.

@bmingles
Copy link
Contributor Author

bmingles commented Oct 8, 2024

Ok, I unpublished the package for now. Sounds like it can be republished as long as it has a new version number after a 24 hour waiting period.
https://docs.npmjs.com/unpublishing-packages-from-the-registry#when-to-unpublish

I'm not 100% sure if anyone can do so or if it is somehow forever tied to my name, but I can always re-publish, re-add owners, etc. if that becomes an issue. It's not listed under my name at all anymore, so I don't expect this to be the case anyway.

@RealAlphabet
Copy link

I don't have any specific feedback on this, but I want to express my gratitude. Thank you for taking the initiative to implement this - it will greatly facilitate the process of porting Slate to frameworks beyond React. I've been following the project for some time and have been eager for a Vue alternative. If this PR is accepted, I’m confident it will pave the way for easier adaptations to other frameworks.

Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

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

ok, I think this is mostly ready.

We need to go through things like tconfig.json and the various release scripts to make sure the new package gets picked up for release.

@bmingles
Copy link
Contributor Author

@dylans Great to hear. Anything I need to do, or is this in maintainers’ court?

@dylans
Copy link
Collaborator

dylans commented Oct 24, 2024

@bmingles well, I am the active lead maintainer... I'm actually waiting to make sure I have npm admin access for the existing repos and then we'll do this release. I hope to have that sorted out soon.

@dylans
Copy link
Collaborator

dylans commented Oct 24, 2024

@bmingles apologies for the merge conflicts with the other PR that landed :(

@bmingles bmingles requested a review from dylans October 25, 2024 02:03
@bmingles
Copy link
Contributor Author

@bmingles apologies for the merge conflicts with the other PR that landed :(

I have rebased the PR

@dylans
Copy link
Collaborator

dylans commented Oct 25, 2024

Thanks, I just need to find time to make sure this publishes to npm with our automated process. I now have npm owner access for the other repos... I'll let you know asap.

@dylans dylans merged commit 9a21251 into ianstormtaylor:main Oct 31, 2024
11 checks passed
This was referenced Oct 31, 2024
@Arkellys
Copy link

Arkellys commented Nov 3, 2024

Shouldn't this have been marked as a breaking change on release [email protected]? I could no longer start my app after updating slate-react to 0.111.0 without installing slate-dom package. Maybe the "Installing Slate" documentation should be updated too.

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.

Split out slate-dom package from slate-react
4 participants