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

registerWebhooks() is taking too long to run #734

Open
nullndr opened this issue Jun 5, 2024 · 7 comments
Open

registerWebhooks() is taking too long to run #734

nullndr opened this issue Jun 5, 2024 · 7 comments

Comments

@nullndr
Copy link

nullndr commented Jun 5, 2024

I noticed the LCP in my app has been raised recently, so after some debugging I found that the call to registerWebhooks() in the afterAuth() hook is taking an exagerate amount of seconds (~10s).

Manually registering all my webhooks topic (I currently have 12 topics) inside a Promise.all() decreased the time to 300ms.

@nullndr nullndr changed the title registerWebhooks() tooks too long to run registerWebhooks() is taking too long to run Jun 5, 2024
@lizkenyon
Copy link
Contributor

lizkenyon commented Jun 5, 2024

Question: Are you doing this manual registration still in the afterAuth hook? or elsewhere?
Trying to understand if the issue is in afterAuth or registerWebhooks

@nullndr
Copy link
Author

nullndr commented Jun 6, 2024

Hi @lizkenyon.

My custom registration is still in the afterAuth() hook:

export const shopifyConfig = shopifyApp({
  ...
  webhooks: undefined,
  hooks: {
    async afterAuth({ session, admin }) {
      console.time("customRegister");
      await Promise.all(topics.map(topic => registerWebhook(topic, session)));
      console.timeEnd("customRegister"); // ~300ms
    }
  },
});

async function registerWebhook(
  topic: WebhookSubscriptionTopic,
  { shop: shopifyDomain, accessToken}: Session
) {
  if(accessToken == null) {
    return;
  }

  const genql = await createGenqlClient({ shopifyDomain, acessToken });

  return genql.mutation({
    eventBridgeWebhookSubscriptionCreate: {
      ...
    },
  });
}

With the registerWebhooks() call:

export const shopifyConfig = shopifyApp({
  ...
  webhooks: {
    ...
  },
  hooks: {
    async afterAuth({ session, admin }) {
      console.time("registerWebhook()");
      await shopifyConfig.registerWebhooks({ session });
      console.timeEnd("registerWebhook()"); // ~9/10s
    }
  },
});

Initially I thought the biggest problem is in the call to runMutation() in the shopify-app-js package that is awaited on each call, but on a second thought it doesn't look enough to slow down with seconds.

Trying to understand if the issue is in afterAuth or registerWebhooks

Also, there seems to be a problem even with the afterAuth() (But probably this should be in a separated issue).

I have my app under the /app route, that imports a React.Context of my shop with a simple Prisma model:

export const findShop = async ({ shopifyDomain }: ShopifyDomain) => {
  const shop = await db.$queryRaw<[Shop]>`
    select * from "Shop" where "shopifyDomain" = ${shopifyDomain};`;
  return shop[0];
};

When I have the call to registerWebhooks() at the start of afterAuth(), this model returns undefined, after a refresh I can see the shop entity correctly. This looks like a problem with the afterAuth(), since the remix loader loads before the registration process I do in the body of afterAuth(), indeed if I run:

async afterAuth({ admin, session }) {
  await Promise.all([
    shopifyConfig.registerWebhooks({ session }),
    installOrUpdateShop(admin, session),
  ]);
}

The model correctly returns the shop entity.

I forgot to mention that my @shopify/shopify-app-remix is 2.8.2, sadly I can't update to 3.0.

@lizkenyon
Copy link
Contributor

Hi yes, could you create a separate issue for the AfterAuth. Just want to make sure we don't lose track of it. Thanks! :)

And then I will triage the registerWebhooks issue and we will look into it.

@nullndr
Copy link
Author

nullndr commented Jun 6, 2024

Excellent, thank you so much!

@nullndr
Copy link
Author

nullndr commented Jun 24, 2024

Hi @lizkenyon , any updates on this?

@lizkenyon
Copy link
Contributor

Hi there 👋

Sorry for the delay in this.

If you haven't already I would recommend checking out App-specific webhook subscriptions. You define the webhook subscriptions in the shopify.app.toml, and Shopify manages creating the subscription instead of registering them in the afterAuth hook.

@nullndr
Copy link
Author

nullndr commented Nov 22, 2024

Hi! I am aware of the that and I already moved some webhooks like shop/update to the .toml file. However the main issue remains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants