Migrate for Meteor 3.0

[jankapunkt]

Continuing from #309

[jankapunkt]

Once CI runs we should release a beta asap, wdyt @StorytellerCZ

[StorytellerCZ]

Merged in from #309 as I couldn't push to the original branch for some reason. There are several breaking changes.

[StorytellerCZ]

@bhunjadi I will be watching your fork if anything comes up. I will release a beta shortly, but given that the tests and lint fail we have to look into that @jankapunkt

[StorytellerCZ]

Lint is working and green again!

[StorytellerCZ]

Published matb33:collection-hooks@2.0.0-beta.2

@jankapunkt run into the Meteor version mismatch issue that you were complaining in the past. I think that might also be what is screwing the tests locally.

[StorytellerCZ]

So I managed to get the tests running with the release flag locally. Still have a lot of them stuck and one error. @jankapunkt I think we need to add script for running the test in GitHub Actions properly. I don't remember now which package would be best for that. While at it we should probably standardize this for all the packages that use TinyTests.

[harryadel]

Now the test suit is running properly, at first it wasn't running due to a missing dependency (jquery) and later on it was getting stuck as an async test was still waiting but nothing was being returned.

cc @StorytellerCZ @jankapunkt @bhunjadi

[StorytellerCZ]

@harryadel perfect! Now we'll merge @bhunjadi latest improvements and then fix remaining tests. We can then release an RC and the last thing remaining will be documentation and migration guide.

[StorytellerCZ]

@bhunjadi changes merged, so now we need to fix any broken tests. Either way I will try to release a new beta today.

[harryadel]

cc @StorytellerCZ @bhunjadi

There was a failing test I had to fix. The tests checks for the removal of documents by counting the number of documents, and something causes multiple entries yet the removed document is gone so the test got reworked to just check for that.

I'm going to attempt a new release if I've the rights, if not then it's on you @StorytellerCZ to have a new release today!

[StorytellerCZ]

Will do!

[StorytellerCZ]

Thanks to @jankapunkt for doing the release!

[brianlukoff]

I'm getting this error from montiapm:agent on startup with the RC (wasn't getting the error with the previous beta):

TypeError: Cannot read properties of undefined (reading 'prototype')
    at hijackNextObject (packages/montiapm:agent/lib/hijack/db/hijack-next-object.js:6:42)
    at hijackDBOps (packages/montiapm:agent/lib/hijack/db/index.js:46:3)
    at packages/montiapm:agent/lib/hijack/instrument.js:53:5
    at initAsync (packages/montiapm:meteorx/src/server.js:40:11)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at /tools/static-assets/server/boot.js:453:5
    at /tools/static-assets/server/boot.js:504:7
    at startServerProcess (/tools/static-assets/server/boot.js:501:3)

[jankapunkt]

The question is now, whether this is to be fixed here or in montiapm:agent @StorytellerCZ @harryadel @zodern

[StorytellerCZ]

I think if everyone is happy with the current state we can go for a final release. Thoughts @jankapunkt @harryadel @brianlukoff ?

[brianlukoff]

Unfortunately I am still getting the Monti APM error above running the RC; the beta 2 release works fine.

[harryadel]

@StorytellerCZ I'd rather that we hold out a bit, not only because of the Monti issue but the latest version was causing errors at our application and downgrading was the only fix. So, we need to wait a bit until more people give this a spin.

[StorytellerCZ]

@harryadel yes, we need to fix the tests as well. I will release another RC with the Meteor 3 final release in versions from.

[StorytellerCZ]

@jankapunkt 2.14 tests are failing. Not a bit issue, but it would be good to have a backward compatibility or figure out why. For myself it is not a release blocker as 3.0 tests pass. So the only real thing that we need to figure out is the MontiAPM issue that @harryadel and @brianlukoff spoke of.

[Sergeant61]

Hello, what is the latest situation, when I try this version 2.16, callbacks are not triggered.

my code:

if (Meteor.isServer) {
  Messages.before.insert(async function (userId, doc) {
    doc.search = doc.search || {}

    if (doc.userId) {
      const user = await Meteor.users.findOneAsync({ _id: doc.userId }, { fields: UserSearchFieldSelector })
      doc.search.user = user
    }

    if (doc.lastAgentUserId) {
      const lastAgentUser = await Meteor.users.findOneAsync({ _id: doc.lastAgentUserId }, { fields: UserSearchFieldSelector })
      doc.search.lastAgentUser = lastAgentUser
    }

    return true
  })
}

[StorytellerCZ]

The tests are failing due to error in the email package which has nothing to do with this project. We might need to wait for Meteor 3.0.3 release before we are able to run tests properly.

[matheusccastroo]

For anyone also facing the montiapm related issue: the problem is that this package is trying to monkey patch the find method to support the hooks, and when doing that, it re-creates the mongo cursor and as such the montiapm:agent can't add it's metrics stuff (because it ends up without the synchronousCursor property).

I just released a version for this package (collection-hooks) without the find hooks, so if you don't use it, it will work for you: https://atmospherejs.com/quave/collection-hooks.

[jankapunkt]

Thank you @matheusccastroo for the hotfix

I think under this circumstance we should make it globally configurable which methods to hook by default. @StorytellerCZ wdyt?

[StorytellerCZ]

I don't know. The issue is with the insert hook which configuration would not really resolve. I'm fine with optimization of hooking only collections which actually have hooks assigned.

[zodern]

The issue with montiapm:agent is more an issue with MeteorX - it's unable to get the synchronous cursor since the cursor it has access to is different than the one the fetch is run on when using collection hooks:

https://github.com/monti-apm/meteorx/blob/bd14ff6df893d18854367a1689e0c0e20e7b8b81/src/mongo-livedata.js#L64-L70

https://github.com/Meteor-Community-Packages/meteor-collection-hooks/blob/8484d71158d9ea48651c54d2c0eb5e4016ad55b4/find.js#L58-L64

I'm not familiar with this package, but a couple ideas are:

• it seems a new cursor is created since the hooks could change the cursor options. Maybe it could re-use the original cursor if the options are not changed
• it could add the new cursor as a property to the original cursor, so it can be accessed by outside code.
• edit: the suggestion by @StorytellerCZ to not hook every collection would probably also fix this since MeteorX creates its own collection

[harryadel]

Guys, I released a new version matb33:collection-hooks@2.0.0-rc.4. Please give it a test run. I tried it in my local application but something is not quite right so I'm still looking into it.

[StorytellerCZ]

So pretty sure now that the latest rc caused my website to fall into infinite loading. Not sure why, but reverting to rc.4 fixed the issue.