MaxNoetzold / y-mongodb-provider

Mongodb database adapter for Yjs
MIT License
47 stars 10 forks source link

v0.1.8 is a breaking change #6

Closed raineorshine closed 10 months ago

raineorshine commented 10 months ago

When I upgrade to v0.1.8 the data that was created in v0.1.7 does not work.

When I wipe the db and create the same data from scratch with v0.1.8, it appears to work correctly. So it seems that the changes introduced in v0.1.8 should have resulted in a major version upgrade (or a 0.x upgrade), not a patch upgrade.

Steps to Reproduce

  1. Install y-mongodb-provider@v0.1.7.
  2. Create a YJS Doc, connect it to MongodbPersistence, and set some data.
  3. Build and run.
  4. Once the data has been created, remove that code and add some code that logs the entire Doc on sync.
  5. Build and run to test that the data loads correctly.
  6. Install y-mongodb-provider@v0.1.8.
  7. Rebuild and run. The data does not load.

Notes

Without a test suite, it's hard to tell exactly what was incompatible. The above steps are what I did with my app to reproduce the issue, but likely a simpler test case could be found. My app uses getYDoc, storeUpdate, clearDocument, getMeta, and setMeta.

Solution

I recommend deprecating v0.1.8 and releasing v0.2.0. A 0.x change will correctly indicate that the upgrade may contain breaking changes.

MaxNoetzold commented 10 months ago

Thanks for letting me know! I'll check it out next week. Seems like there might be a bug because I tried adding the feature without messing up the old versions.

Additionally, I should totally throw in a test suite for this project too.

MaxNoetzold commented 10 months ago

Hey, I wasnt able to replicate your problem.

I create a MongodbPersistence instance like that:

const mdb = new MongodbPersistence(createConnectionString('yjstest'), {
    collectionName: 'transactions',
    flushSize: 100,
    multipleCollections: true,
});

First I created ydoc with some data with v0.1.7:

const createDoc = () => {
    const ydoc = new Y.Doc();
    ydoc.on('update', async (update) => {
        mdb.storeUpdate(docName, update);
    });

    const yText = ydoc.getText('name');
    yText.insert(0, 'Test');
    console.log('Inserted');
};

Then I tried to read the data with v0.1.8:

const getDoc = async () => {
    const persistedYdoc = await mdb.getYDoc(docName);

    const yText = persistedYdoc.getText('name');
    console.log('Retrieved', yText.toString());
};

And it logged the expected Retrieved Test.

raineorshine commented 10 months ago

Thanks for looking into it. I have multipleCollections: false fwiw.

I don't see anything not working when I wipe the database and start fresh. Luckily the app is not in production. I'll let you know if anything comes up though.

MaxNoetzold commented 10 months ago

I did try it with multipleCollections: false too. It's unfortunate that I couldn't replicate your problem.

I did manage to add some tests, but honestly, I'm not sure how to include this error case as a test. Would you mind taking a quick look at it?

raineorshine commented 10 months ago

Wow! That’s amazing. Well done. Those tests will be invaluable for any new features or major refactors.

Given that everything is working correctly for me on the new version, I can't justify the time of narrowing the upgrade issue down to an isolated test case. It could be a timing issue, a MongoDB driver issue, a connection string issue, an issue with my MongoDB instance, an issue with any one of the many places I access the db, etc. However if any problems arise I will let you know.

MaxNoetzold commented 10 months ago

Alright. Thank you!

I'll close the issue for now.