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

Migration 3.0 #13

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

klablink
Copy link

@klablink klablink commented Oct 2, 2023

Porting to Meteor version 3.0. The package is not backward compatible because it replaces kernel components that are now asynchronous. Tests can only be performed with packages compatible with version 3.0. All files are migrated to standard.js style.

@micktaiwan
Copy link

Hello, does someone know when this can be merged?

@plbkbx
Copy link

plbkbx commented Jan 17, 2024

It is not yet usable due to problems with the new asynchronous package loading logic. I am waiting for information about this so I can update the package.

@ToyboxZach
Copy link

I have this loaded locally and at least in initial tests its seems to work. I did this: https://guide.meteor.com/writing-atmosphere-packages.html#overriding-atmosphere-packages and then just had to mess around with the requirements.

@plbkbx is there a particular bug you are seeing? I'm curious why my case seems to work and yours doesn't.

I do do my RedisOplog.init inside of a Meteor.startup call which might help resolve the loading order issue you are talking about?

I'm not entirely sure what about RedisOplog matters if stuff is loaded out of order.

@plbkbx
Copy link

plbkbx commented Jan 19, 2024

For an explanation of the issue I send you to https://forums.meteor.com/t/order-of-loading-packages/61112/9. The problem is insidious and could occur the particular cases.

@ToyboxZach
Copy link

Right, I get that packages won't load in the right order, but looking at the code of this package I'm not entirely sure why that matters.

Do you have an example repo showing redis-oplog not working correctly? It could be helpful if anyone makes an update to this package.

@plbkbx
Copy link

plbkbx commented Jan 19, 2024

This package does a Monky Patch of the Mongo.Collection constructor to register all the instances created (mongoCollectionNames.js). If another package is run first and creates a collection it is not recognized and generates an error.

I don't have an example with redis-oplog but I generated a simple one to verify the problem.

@ToyboxZach
Copy link

Ah ok, so not really a problem with this package at all, but more a Meteor issue. I don't think there is anyway for this package to resolve that problem, maybe it could give an option to convert and register the collection after the fact.

For people, like me who don't need redis to be connected to any collections from other packages I think this will work just fine, so in theory this PR could go through. I don't have the expertise in it though to know all the possible use cases that would need to be checked.

.github/workflows/test.yml Outdated Show resolved Hide resolved
package.js Outdated Show resolved Hide resolved
@StorytellerCZ
Copy link
Member

Published cultofcoders:[email protected] with the latest from this branch for user testing targeting 3.0-rc.4 for now.

@StorytellerCZ
Copy link
Member

I don't have permissions to push to this branch, but I only adjusted package.js

@JanMP
Copy link

JanMP commented Jul 18, 2024

I got this error with 3.0-rc.1 under meteor 3.0.1
The problem seems to be related to wildhart:jobs

Jobs Meteor.startup, startupDelay: 1s...
RedisOplog - Established connection to redis.
Jobs Jobs.run syncArticles { in: { minutes: 1 }, singular: true }
Jobs Jobs.run syncRecipes { in: { minutes: 2 }, singular: true }
Jobs Jobs.run syncConfluence { in: { minutes: 3 }, singular: true }
Jobs   Singular job already exists
Jobs   Singular job already exists
Jobs   Singular job already exists

meteor://💻app/packages/cultofcoders_redis-oplog.js:474
          throw new Meteor.Error("We could not find the collection instance by name: \"".concat(this.collectionName, "\", the cursor description was: ").concat(JSON.stringify(cursorDescription)));
                ^
errorClass [Error]: [We could not find the collection instance by name: "jobs_dominator_3", the cursor description was: {"collectionName":"jobs_dominator_3","selector":{"_id":"dominatorId"},"options":{"transform":null}}]
    at new ObservableCollection (packages/cultofcoders:redis-oplog/lib/cache/ObservableCollection.js:41:13)
    at new RedisOplogObserveDriver (packages/cultofcoders:redis-oplog/lib/mongo/RedisOplogObserveDriver.js:38:33)
    at MongoConnection._observeChanges (packages/cultofcoders:redis-oplog/lib/mongo/observeChanges.js:131:21)
    at Cursor.observeChanges (packages/mongo/mongo_driver.js:982:22)
    at Function.LocalCollection._observeFromObserveChanges (packages/minimongo/local_collection.js:1666:25)
    at Cursor.observe (packages/mongo/mongo_driver.js:954:26)
    at initAsync (packages/wildhart:jobs/jobs.ts:47:40)
    at packages/wildhart:jobs/jobs.ts:39:27
    at EnvironmentVariableAsync.<anonymous> (packages/meteor.js:1292:16)
    at packages/meteor.js:763:17
    at AsyncLocalStorage.run (node:async_hooks:346:14)
    at Object.Meteor._runAsync (packages/meteor.js:760:14)
    at EnvironmentVariableAsync.withValue (packages/meteor.js:1288:19)
    at packages/meteor.js:502:25
    at packages/meteor.js:1399:24
    at packages/meteor.js:763:17 {
  isClientSafe: true,
  error: 'We could not find the collection instance by name: "jobs_dominator_3", the cursor description was: {"collectionName":"jobs_dominator_3","selector":{"_id":"dominatorId"},"options":{"transform":null}}',
  reason: undefined,
  details: undefined,
  errorType: 'Meteor.Error'
}

Node.js v20.15.1

@StorytellerCZ
Copy link
Member

rc.2 is from another branch. If that one works, we will move to that branch.

@JanMP
Copy link

JanMP commented Jul 18, 2024

rc.2 has the same problem. I also tested it without wildhart:jobs. I get the same error with 'users' instead of 'jobs_dominator_3' then.

@StorytellerCZ
Copy link
Member

Interesting.

@brianlukoff
Copy link

It seems like client-side updates to the profile field of Meteor.users no longer works with redis-oplog with this branch -- the server does execute the update but the client isn't notified of the change. Has anyone else seen this?

@StorytellerCZ
Copy link
Member

@klablink can you grand me access to your fork so that I can push updates? Thanks!

Also can we potentially merge this with this PR: cult-of-coders#405
We have already published some RCs from that branch, so it would be nice to merge into that.

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.

8 participants