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.

Sign up for GitHub

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

Jump to bottom

[WIP] Meteor v3 #309

Open
wants to merge 17 commits into
base: migrate/3.0
Choose a base branch
from
Open

[WIP] Meteor v3 #309

wants to merge 17 commits into from

Conversation

bhunjadi
Copy link

@bhunjadi bhunjadi commented Feb 25, 2024 •
edited

I started working on this since I was stuck when updating grapher (PR: cult-of-coders/grapher#484)

I have one test failing right now for Meteor v3. I'll also have to test with Meteor v2.

Current state:

  • directOp and hookedOp return promise because EnvironmentVariable.withValue returns promise
  • Meteor's collection.remove doesn't support a callback -> collection.removeAsync must be used even on the client
  • Meteor's allow-deny will call _validateUpdate when doing collection.update which means that findOne will be used on server when calling the method from client -> collection.*Async must be used on client, too
  • Advices are now async functions, we might not want this since it's not backward compatible, I'll try to figure that out. Maybe we return promises on server and values on client.

Other comments:

  • Cannot run --once with TinyTest, it just hangs waiting with App running at: http://localhost:3025/
  • Testing with Meteor 2.15 for v2. I still have 8 errors that I have to check. We won't support Meteor 2 with this release.

[Update]
I cannot figure out failing test for Meteor 3.0. Maybe it might be a bug there: meteor/meteor#13036

[Update 2]
Fixed the test for v3, it appears it is expected behavior in Meteor v3, but might be fixed in the future (meteor/meteor#13052 (comment))

[Update 3]
Made all before and after hooks async except find(), meaning that we'll be awaiting them before calling the original method or before returning results.

Basically:

function wrapperOfX(...) {
   await beforeHooks(...);
   const result = await X(...);
   await afterHooks(...);
   return result;
}

Before and after hooks are triggered in order they were added. This reflects the old behavior.
If this is not desired, we could trigger them simultaneously or add an option for caller to define how should they be handled - all at once or one by one.

find() has its own problems, if we allow before/after hooks directly it means we'll have to await find() which we cannot do. Alternatives are patching (wrapping) returned cursor methods (fetchAsync, countAsync) and use the after/before hooks there.

There are also some issues with Meteor v2.13 tests that I have to figure out.

The problem with Meteor v2 and async approach is that in accounts-base/accounts_server.js we have this code

async _attemptLogin(
    methodInvocation,
    methodName,
    methodArgs,
    result
  ) {
    ....
    let user;
    if (result.userId)
      // If this was await this.users.findOneAsync it would work
      // however, calling findOne returns a promise now since we're using async after/before
      user = this.users.findOne(result.userId, {fields: this._options.defaultFieldSelector});

Also, I cannot make meteor test-packages --once work at all.
This is the result:

meteor test-packages --release 3.0-rc.0 --port 3025 --once ./
[[[[[ Tests ]]]]]                             

=> Started proxy.                             
=> Started MongoDB.                           
=> Linted your app. No linting errors.        
=> Started your app.                          

=> App running at: http://localhost:3025/

I20240423-20:57:28.478(2)? (temp) test passes: true

bhunjadi
bhunjadi commented Feb 26, 2024
View reviewed changes
tests/direct.js Outdated Show resolved Hide resolved
@bhunjadi bhunjadi mentioned this pull request Feb 26, 2024
Open
@bhunjadi
Copy link
Author

@StorytellerCZ

I'm have this wrapped up for Meteor 3 just have to do some minor updates to before and after callbacks being async, but there are some important things to discuss before I continue to work on this.

I hope we agree that after & before should now be async functions, as discussed here: #308

Then we have this issue about async/sync methods: #302

If before/after hooks are async, that means that all wrappers that this package will provide will be async, too.
That works when client/server expect use async functions, but what about sync version (on Meteor 3.0 only relevant client-side)?

For example, with async insert wrapper we have this issue

if (Meteor.isClient) {
  Tinytest.onlyAsync('insert - local collection sync insert', async function (test, next) {
    const collection = new Mongo.Collection(null)
    const tmp = {}

    collection.before.insert(function (userId, doc) {
      tmp.typeof_userId = typeof userId
      doc.before_insert_value = true
    })

    InsecureLogin.ready(function () {
      // Since insert.js is now async because it supports before/after being async, we wrap the sync insert()
      // method but return the promise!
      const res = collection.insert({ start_value: true }, (err, res) => {
        // This works ok
        test.equal(typeof res, 'string', 'Callback value must be string')
        console.log('res', res)
        next()
      })
      // This fails, return value is a promise.
      test.equal(typeof res, 'string', 'Return value must be string')
    })
  })
}

We obviously cannot do that since it changes Meteor's interface.

Some options that came to my mind

Option 1: separate sync and async callbacks

We could keep sync and async versions of all methods (insert, update, etc) and have sync & async versions of before and after callbacks.

const collection = new Mongo.Collection(null)

collection.before.insert(function syncFn() {});
collection.before.insertAsync(async function() {});

// Uses only sync callbacks
collection.insert({});

// Uses both sync & async callbacks
collection.insertAsync({});

Good:

  • v2 compatibility will be easier to achieve, although it won't be 100%

Bad:

  • Sync calls won't be fully fledged since async callbacks won't fire.
  • Might be confusing to use, hard to debug
  • Also, as mentioned by @klablink , it will be harder to cover all combinations with test cases and we'll probably be left with some code duplication.

Option 2: hooks to run only on async calls

Good:

  • Simple (to implement)

Bad:

  • No backward compatibility, unexpected behavior for sync calls

Let me know if you have other ideas on how to solve this.

@vparpoil
Copy link
Contributor

vparpoil commented Mar 27, 2024 •
edited

In my opinion, option 2 (providing hooks only for async versions) is the way to go, as sync versions of the collection calls should disappear very quickly of our apps as we migrate to meteor 3

@pmogollons
Copy link

Option 2 will be better, but also allowing hooking sync methods. This will make it not backwards compatible, but in a more compatible way while there is migration to async methods. This will also fix #301.

@harryadel
Copy link
Member

harryadel commented Apr 1, 2024 •
edited

Yeah, I'd prefer option 2. What's the status of this PR? Can you include the option and release a beta soon?

We actually chose to do something similar for both ostrio:files and collection2 completely dropping support for sync functions.

cc @StorytellerCZ @bhunjadi

@bhunjadi
Copy link
Author

bhunjadi commented Apr 1, 2024

Ok, looks like it will be option #2. The last question if whether we rename the methods to *Async or leave them as is?

I think I can have this ready for start of next week.

@manueltimita
Copy link

@bhunjadi Great work!

+1 from me too for option 2.

@dokithonon
Copy link

Hello, I am testing meteor 3 rc.0 with collection hook, they are not triggered : example repo : https://github.com/dokithonon/meteor-test-async/tree/3.0-rc.0

Is this PR will fix this ? Thanks a lot

@bhunjadi
Copy link
Author

Going with option #2 seems reasonable. There is one problem, though. find should return a cursor, but if we want to support async before & after hooks the current idea won't work as it would have to return the promise from find() which would in turn change Meteor's API.

Patching a cursor returned from find() is not ideal.
Any thoughts on how to solve this?

@dokithonon Yes, this PR should fix it.

@StorytellerCZ
Copy link
Member

Option 2 it will have to be. The use case is that the hook is tied to action, not to a/sync execution. For Meteor 3, the sync executions can be left only to the client as on server everything is async.

@bhunjadi
Copy link
Author

bhunjadi commented May 8, 2024

I'll have to do some updates regarding the sync functions that work on the client. Currently we hook into them also and they become async which is not desired.
That means hooks won't work on sync functions with that implementation, unless we want to add sync hooks for that use case.

@StorytellerCZ
Copy link
Member

Alright! This has become a major blocker for me in Meteor 3 adoption, but I'm just starting to learn about the code, still let me know if I can help in any way. Also I'll be happy to publish any updates under the beta tag.

@kolyasya
Copy link

@bhunjadi hey, thank you for your work.
I'm trying to test v1.4.0-beta.1 in my project, but don't see any evidence that before.insert hook works.

What is the current state?

@harryadel
Copy link
Member

I don't want to back-pedal on my promise but I'm looking into this package right now and figuring out how to continue @bhunjadi great work, wish me good luck boys 😄

@bhunjadi
Copy link
Author

Just to clarify that I'll probably be able to invest some of my time this week into this package.
I was not aware beta version was already published.

@harryadel
Copy link
Member

harryadel commented May 21, 2024 •
edited

@bhunjadi I know how intensive and underappreciated open source work can be yet this package needs to be made async asap so would you be able to dedicate more time even if if you weren't able to wrap it up this week? Basically we need someone dedicated to fix this problem and not one and off.

Let me assert again this isn't to undermine your efforts of what you've done so far. You've done great work that we all look up to and appreciate. It's just we need a dedicated person time-wise to wrap this up and get it over with.

EDIT: I'll probably carry it over from here regardless as this is blocking our app update!

@bhunjadi
Copy link
Author

Ok, I have some uncommited changes and I'll try to figure out if I can push these. You can then merge these if there woould be a need to do it.

So, to summarize the issues I'm aware of:

  • tests are not working with --once, looks like an issue with tiny which might be fixed in the meantime (if not, mocha + meteortesting should work)
  • sync versions of methods on client are currently async (returning promise): I think the conclusion was that hooks shouldn't run for these
  • problem with find hook, we cannot return Promise instead of Cursor because it will break everything

@kolyasya
I just tried v1.4.0-beta.1 and when using the debugger I don't see anything from this branch so I'd say this is no different than 1.3.1 functionality-wise.
Maybe @StorytellerCZ can provide more info on what was published in that beta.

@StorytellerCZ
Copy link
Member

We can do a v2 with breaking changes for Meteor 3. These would be that we won't deal with sync collections calls on the client and we can drop support for the find hook due to the breaking changes in async in Meteor 3.
I think we should position ourselves here to keep things going for the moment until there is a solution for collection hooks in core which was stated as an intention and I will be pushing for that.

@StorytellerCZ
Copy link
Member

As for the beta that is from the other branch which only help you to resolve the dependencies when upgrading to Meteor 3, nothing else really.

@kolyasya
Copy link

@bhunjadi yeah, sorry, with the files from PR hooks seem to work for me.

@bhunjadi
Copy link
Author

@StorytellerCZ

I pushed the changes related to our discussions above. The tests are passing locally in watch mode, but when using --once they're still stuck so I expect the pipeline will fail.

Can we publish this as 2.0.0 alpha maybe?

Breaking changes

  1. Hooks are executed only for Async methods because we're supporting async hooks only right now. That means that, for example, collecdtion.insert() won't trigger any hooks.
  2. As Collection.find returns cursor, we cannot execute async hooks for that one, either.
  3. Dropped support for Meteor 2

For 1) we can introduce sync hooks.
For 2) I'd like to figure out how people are using this. If introduction of sync hooks would suffice, then it could be resolved with solution for #1.

  1. shouldn't be a problem

@harryadel
Copy link
Member

harryadel commented May 22, 2024 •
edited

@bhunjadi

As Collection.find returns cursor, we cannot execute async hooks for that one, either

What do you mean by "breaking" cannot the cursor values be promisified so later processes can be run on it?

@bhunjadi
Copy link
Author

If we just re-use the logic from before, find() method will become async, meaning it would have to be used like await (await coll.find()).fetchAsync())

If you look at the code, find() wrapper allows hooks to change selector and options and then passes these modified values into the wrapped find().

One possible workaround I can think of is that we patch the cursor's fetchAsync, countAsync and so on.
find wrapper would then look like this

const cursor = _super.call(this, selector, options)
patchCursor(cursor)
return cursor

patchCursor would probably wrap around fetchAsync and countAsync and alter cursor's _cursorDescription in that case (ref: https://github.com/meteor/meteor/blob/a89870aed294f21d3cb23bf16fb8c5b1b2a48a78/packages/mongo/mongo_driver.js#L894)

@dokithonon
Copy link

dokithonon commented May 23, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@bhunjadi @vparpoil @pmogollons @harryadel @manueltimita @dokithonon @StorytellerCZ @kolyasya