meteor / meteor

Meteor, the JavaScript App Platform
https://meteor.com
Other
44.29k stars 5.17k forks source link

Meteor 3: Error: Meteor.userId can only be invoked in method calls or publications with Meteor.bindEnvironment #13258

Open aboire opened 1 month ago

aboire commented 1 month ago

Hello,

I am encountering an issue while updating an application from Meteor 2 to Meteor 3. In my application, I use Meteor.userId() within helpers and other functions wrapped with Meteor.bindEnvironment.

Under Meteor 2, everything worked correctly. However, after updating to Meteor 3, I receive the following error when trying to use Meteor.userId() within Meteor.bindEnvironment:

Error: Meteor.userId can only be invoked in method calls or publications.

New Findings

The issue appears in the publish-composite package, where Meteor.bindEnvironment is used in the publication.js file. This usage causes Meteor.userId() to fail.

manueltimita commented 1 month ago

This is interesting and it requires further investigation. I am quite familiar with this package and I make extensive use of it.

Currently on Meteor v3.0.1 and reywood:publish-composite@1.8.9, so I ran a quick test. It looks like this error is emitted only when Meteor.userId() is called in one of the children. Everything seems fine if called in the main body of the publication, or in the top level find().

I see that @StorytellerCZ has added this issue to the Release 3.x.x milestone. Jan, if you know more at this stage, is this a Meteor issue, or something to be looked at in publish-composite?

StorytellerCZ commented 1 month ago

I don't know. Haven't looked into it, but if the issue is that you can't call Meteor.userId() in the children of publish-composite, then I would think that the issue is with publish-composite and the workaround is to call this on the top.

aboire commented 1 month ago

For me the problem comes from calling Meteor.userId() in Meteor.bindEnvironment

In a standard publish it's the same

if you put this code in a publish

const test = Meteor.bindEnvironment(async () => {
console.log('bindEnvironment Meteor.userId', Meteor.userId());
});
await test();

I have this error

Error: Meteor.userId can only be invoked in method calls or publications.

DDP._CurrentPublicationInvocation.get() is undefined inside Meteor.bindEnvironment

mcorbelli commented 3 weeks ago

Some news?

nachocodoner commented 2 weeks ago

I’m testing the behaviors mentioned in this issue as we plan to address it next.

However, I can't identify the behavior that differs from Meteor 2 compatibility. As shown in the picture, I ran Meteor 2 and 3 with the same code and logged the Meteor.userId() in three scenarios: directly, via an async function, and via an async function with Meteor.bindEnvironment. The first two scenarios worked fine, but Meteor.bindEnvironment failed in BOTH Meteor 2 and 3.

How can I specifically identify the expected behavior in Meteor 3 compared to Meteor 2? Supporting this should be considered a new feature, not a regression, with Meteor 3.

image

aboire commented 2 weeks ago

No it's different between meteor 2 and 3 on this:

Meteor.publish('TestBindEnvironment', async function () {

  if (!this.userId) {
    return null;
  }
  const test = Meteor.bindEnvironment(async () => {
    console.log('bindEnvironment Meteor.userId', Meteor.userId());
  });
  await test();

  return this.ready();
});

you have no difference because you put Meteor.bindEnvironment outside the publish

aboire commented 2 weeks ago

on meteor 2 : is ok DDP._CurrentPublicationInvocation.get() on meteor 3: DDP._CurrentPublicationInvocation.get() is undefined

Meteor.publish('TestBindEnvironment', async function () {

  if (!this.userId) {
    return null;
  }
  const test = Meteor.bindEnvironment(async () => {
    console.log('bindEnvironment _CurrentPublicationInvocation', DDP._CurrentPublicationInvocation.get());
  });
  await test();

  return this.ready();
});

that's why I have an error on publishComposite on meteor 3 https://github.com/Meteor-Community-Packages/meteor-publish-composite/blob/111ac1c1b421809ed051b27c17bea23009a3fd22/lib/publication.js#L35 and not on meteor 2 when I use Meteor.userId() in a child find

nachocodoner commented 2 weeks ago

@aboire: Thank you for the clarification. However, I am still seeing same behavior working in my Meteor 2 and Meteor 3 app instances. It doesn't show any error, and in both, I can see DDP._CurrentPublicationInvocation.get() output as expected.

image

Using Meteor 2.16 and Meteor 3.0.2 versions for a simple app with basic setup.

nachocodoner commented 2 weeks ago

I also added publish-composite to ensure it doesn't interfere with the publication logic. I can't seem to get into the error you get.

Could any other packages on your list affect the publish behavior and cause this issue indirectly? Could you provide a list of packages or a reproducible repository? Alternatively, you can debug by removing extra packages to isolate the problem.

aboire commented 2 weeks ago

@nachocodoner Thank you for taking the time to help me

I performed a test without using any packages, and as you mentioned, I did not encounter any errors. I then conducted two simple tests using only the publication of tests on Meteor 2.16 and Meteor 3.0.2. After that, I added my packages, and with these packages, the error appeared. I investigated which package could be causing the issue by disabling each one progressively. I finally identified that the universe:i18n package was the source of the problem. Upon examining the package’s source code here, I noticed that if I comment out this line, DDP._CurrentPublicationInvocation.get() correctly returns a value in my test.

aboire commented 2 weeks ago

I just noticed that two packages are causing issues for me: universe:i18n and matb33:collection-hooks (version 2.0.0-rc.2). The latter package could also be contributing to the problem. Here is the link to the relevant file in the package: https://github.com/Meteor-Community-Packages/meteor-collection-hooks/blob/migrate/3.0/server.js.

nachocodoner commented 1 week ago

Nice catch! Those packages that change publish behavior are indeed causing issues in Meteor 3. I have identified the breaking change.

Meteor 3 introduced changes to the Environment Variable used to create data contexts in Meteor flows, ensuring proper behavior. This breaking change seem to be introduced in this PR: https://github.com/meteor/meteor/pull/13063, where variables are cleaned outside the context where the Environment Variable is wrapped. cc @denihs

A fix that worked for me involves using EnvironmentVariable.withValue before calling publish.call. This ensures the context is preserved and spread properly throughout the entire flow of an altered publication. Here's an example of the fix:

function patchPublish(publish: typeof Meteor.publish) {
  return function (this: typeof Meteor, name, func, ...args) {
    return _publishConnectionId.withValue(this?.connection?.id, () => {
      return publish.call(
        this,
        name,
        function (...args) {
          return func.apply(this, args);
        },
        ...args,
      );
    });
  } as typeof Meteor.publish;
}

Original code from: https://github.com/vazco/meteor-universe-i18n/blob/master/source/server.ts#L221-L234

I'm unsure if this change might cause other test failures in the meteor-universe-i18n package. We should open a PR in the all affected packages that alter publish behavior to provide the fix if required and verify that all tests still pass.

image