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

Rewrite of the Redis extension #203

Closed
wants to merge 79 commits into from
Closed

Rewrite of the Redis extension #203

wants to merge 79 commits into from

Conversation

tommoor
Copy link
Collaborator

@tommoor tommoor commented Sep 22, 2021

Very much work in progress, but opened so others know work is ongoing 😄

TODO

  • Tests for onPersist
  • Tests for onChange
  • Documentation
  • Query awareness from other servers on subscribe
  • Attach server guid to update source and filter out messages from the same server to prevent loops when applying sync steps
  • PubSub subscription should not block onCreateDocument
  • Plan for requiring code from server/src cc @hanspagel

closes #178

@hanspagel
Copy link
Contributor

OMG, nice, thanks for kicking this off!

@tommoor
Copy link
Collaborator Author

tommoor commented Sep 23, 2021

How to require code from server/src

Could use your help here on structuring @hanspagel . This extension relies a lot on stuff in server.

PubSub subscription should not block onCreateDocument

In order to achieve this I think I need some sort of hook or callback when all onCreateDocument hooks have fired and the doc is considered "loaded". Right now I'm relying on it being loaded when passed to onCreateDocument for PubSub extension which would mean it must be placed last in extensions array.

@hanspagel
Copy link
Contributor

Oh, you’re message got somehow lost in my notifications.

I tried to load code from other packages but failed. It’s only possible with hacky workarounds. I only the two solutions:

Moving something to the shared folder. Or duplicate code.

We can add the concept of priorities to make sure extensions are loaded in the right order (if that’s needed). This exists in ProseMirror/tiptap and works fine.

Or we could introduce a onDocmentCreated hook.

What do you prefer?

@hanspagel
Copy link
Contributor

I don’t like the name. Maybe @hocuspocus/redis-scaling? Not sure. Don’t rename it yet.

@tommoor
Copy link
Collaborator Author

tommoor commented Sep 26, 2021

Moving something to the shared folder. Or duplicate code.

Could we use an alias in development so @hocuspocus/server points to the folder, but in production it will use the real package?

Or we could introduce a onDocmentCreated hook.

I added onCreatedDocument for now, btw I think it's time to rename onCreateDocument before too many people use this package. onRequestDocument or onLoadDocument maybe.

I don’t like the name.

With some small refactoring I can make the pubsub backend pluggable, worth bearing in mind.

@tommoor
Copy link
Collaborator Author

tommoor commented Oct 5, 2021

I tried to have the tests understand the aliases in tsconfig – this seems like the best way forward for the future, but it seems like tsconfig-paths does not support esm so we're between a rock and a hard place.

It would probably be easiest to remove the esm requirement.

@hanspagel
Copy link
Contributor

That would hurt. Or we’re using that symlink hack and hope to get rid of it in the future?

@thegoleffect
Copy link
Collaborator

With the latest TypeScript beta, users should be able to use custom loaders in order to get tsconfig-paths to support ESM without any changes on their side: dividab/tsconfig-paths#122 (comment)

Would just need to handle all the annoying node.js idiosyncrasies regarding importing index.[tj]s, different file extensions, and folders.

@tommoor
Copy link
Collaborator Author

tommoor commented Oct 17, 2021

I think the work updating the tests to understand the modules should be done in it's own PR and this PR can be considered blocked by it, which is truly unfortunate as it's so close.

@hanspagel
Copy link
Contributor

I’ll take care of it this week ✌️

@hanspagel hanspagel changed the title wip: PubSub extension Rewrite of the Redis extension Jan 6, 2022
@hanspagel
Copy link
Contributor

hanspagel commented Jan 12, 2022

Made good progress, had to switch to a new branch (because I’m an idiot) though. I’d like to do or at least look into the following, before I file a new PR:

  • Use the MessageReceiver from the server, instead of the local MessageReceiver?
  • Write a migration guide from the old Redis extension to the new one (from the developers perspective, not that much has changed, I’ll write down additional information based on feedback)
  • Add support for connection strings, and config objects (don’t know how the API would look like, but everything can be passed as additional options)

@tommoor
Copy link
Collaborator Author

tommoor commented Jan 12, 2022

Looks like I have lots to catchup on 😅 – glad you're working on it!

@hanspagel
Copy link
Contributor

Closing this here in favor of #285, which is based on that branch here. Details follow. Will release very soon.

@hanspagel hanspagel closed this Jan 15, 2022
@janthurau janthurau deleted the tom/pubsub-178 branch February 7, 2023 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Redis Scaling
3 participants