Shopify / shopify-app-js

MIT License
213 stars 86 forks source link

[shopify-app-remix] Managed installation and parallel route loading #1057

Open zirkelc opened 2 weeks ago

zirkelc commented 2 weeks ago

Issue summary

Hi,

I'm not sure if this is strictly a bug or if I simply need some advice how to handle the new managed installation flow correctly. I'm using the default remix template created by the Shopify CLI. After the client has installed the app through managed installation, Shopify redirects the client to the app on route /app. Since it's the first request after installation, the shopifyApp() starts the Authenticating admin request flow and creates a new session and stores it on the session storage. This process is executed multiple times by each loader() function matching the current route. In this case, there are two loaders being executed for the request /app:

Both loaders run in parallel due to Remix's parallel route loading. Then, after Authenticating admin request has finished the afterAuth() hook is called. This hook would actually be called twice because of each loader. However, the managed installation via the TokenExchangeStrategy dedupes this call so it only runs once:

https://github.com/Shopify/shopify-app-js/blob/7045fe4e3a9c39b858ca8e8337978ee42d496b82/packages/apps/shopify-app-remix/src/server/authenticate/admin/strategies/token-exchange.ts#L159-L172

I use the afterAuth hook to create a database entities (user, shop, etc) in my own system. All other authenticated routes need this information and query this API in their respective loaders. Now the problem is that the managed installation through TokenExchangeStrategy doesn't provide a sequential authentication flow like with the previous AuthCodeFlowStrategy. The authentication in a managed installation seem to be started by multiple loaders in parallel, and the "fastest" loader calls the afterAuth hook. That means there are multiple routes rely on the information being created in the afterAuth hook but it may not be created yet. So I end up with lots of not found errors in loaders because they run before the afterAuth has finished. The errors disappear after a reload.

My first idea was to throw a redirect at the end of afterAuth to force the entire route to reload when the entity was created. But redirect is not exposed on the params of afterAuth and any error thrown inside afterAuth it caught by an outer try-catch anyway.

What is the right way to handle this issue? I think doing API calls inside afterAuth after installation sounds like a normal thing, right?

Before opening this issue, I have:

Expected behavior

After the managed installation, all loaders should wait for the afterAuth hook to be finished before returning.

Actual behavior

The -> app.tsx loader and -> app._index.tsx loader are started in parallel. Both loaders start the Authenticating admin request flow. The first loaders calls the -> afterAuth hook while the second loader finishes with <- app._index.tsx loader before <- afterAuth and <- app.tsx loader have finished.

14:45:48 │ remix         │ [shopify-api/INFO] version 11.0.0, environment Remix
14:45:48 │ remix         │ -> app.tsx loader
14:45:48 │ remix         │ -> app._index.tsx loader
14:45:48 │ remix         │ [shopify-app/INFO] Authenticating admin request
14:45:48 │ remix         │ [shopify-app/INFO] Authenticating admin request
14:45:48 │ remix         │ [shopify-app/INFO] No valid session found
14:45:48 │ remix         │ [shopify-app/INFO] Requesting offline access token
14:45:48 │ remix         │ [shopify-app/INFO] No valid session found
14:45:48 │ remix         │ [shopify-app/INFO] Requesting offline access token
14:45:49 │ remix         │ [shopify-api/INFO] Creating new session | {shop: nebula-etl-test.myshopify.com, isOnline: false}
14:45:49 │ remix         │ [shopify-app/INFO] Running afterAuth hook
14:45:49 │ remix         │ -> afterAuth
14:45:49 │ remix         │ Creating user
14:45:49 │ remix         │ [shopify-api/INFO] Creating new session | {shop: nebula-etl-test.myshopify.com, isOnline: false}
14:45:49 │ remix         │ <- app._index.tsx loader
14:45:49 │ remix         │ No user found
14:45:50 │ remix         │ User created
14:45:50 │ remix         │ [shopify-api/INFO] Registering webhooks | {shop: nebula-etl-test.myshopify.com}
14:45:50 │ remix         │ <- afterAuth
14:45:50 │ remix         │ <- app.tsx loader

Steps to reproduce the problem

Repo: https://github.com/zirkelc/shopify-managed-installation

pnpm shopify app dev

paulomarg commented 1 week ago

Hey, thanks for raising this. Unfortunately, because Remix calls loaders in parallel and they don't share context, there's no way to guarantee their order, or to have them wait for one another. However, it should still be possible to achieve what you want, even if it is a bit more verbose.

The way the template is set up, any route under /app uses, as you pointed out:

The main reason we set it up that way was because the layout makes it easier to add more routes, but the layout isn't absolutely necessary, so you can authenticate any route, even if it isn't under /app.

You should be able to load a route with a single loader if that route:

I don't know the structure of your routes exactly, but I would imagine you don't have to do this for every one of them, just the ones that will be accessed directly (e.g. the home page or any deep link for a partner coming from outside the app), when a user would actually be created.

[!TIP] If you need to change your home page's route, you should also change this redirect call

Hope this helps!

zirkelc commented 1 week ago

Hi @paulomarg thank you for taking the time to respond!

I'm not sure I understand your suggestion completely. Let me try to summarize it:

After an app was approved to be installed via the managed installation flow, Shopify redirects the merchant to the app. What is the target URL after in this case? I assume it's the application_url in the shopify.app.toml (same asappUrl on shopifyApp()), right?

The route (and loader) which is active on this application_url route must authenticate the request and trigger the afterAuth hook to create the user in the database. Then, its should redirect to any other routes that assumes the user already exists. So the key is that only one route (loader) after installation is active.

Is this what you meant?


On a sidenote, what is the purpose of the _index/route.tsx and who calls this route?

paulomarg commented 1 week ago

After an app was approved to be installed via the managed installation flow, Shopify redirects the merchant to the app. What is the target URL after in this case? I assume it's the application_url in the shopify.app.toml (same asappUrl on shopifyApp()), right?

Right!

The route (and loader) which is active on this application_url route must authenticate the request and trigger the afterAuth hook to create the user in the database. Then, its should redirect to any other routes that assumes the user already exists. So the key is that only one route (loader) after installation is active.

Right again. What happens here is that the template runs 2 loaders in this case - one in routes/app._index.tsx, and one in routes/app.tsx - the second one is a Remix layout that is run for every route under routes/app.*.tsx.

What I meant was that you don't need to have a layout, so you could for instance delete routes/app.tsx, and the routing would still work, and only the loader in routes/app._index.tsx would be run.

That would probably solve the problem you're having with 2 loaders running for that action. The downside is that you would need to replicate the code from routes/app.tsx in every route in routes/app.*.tsx for authenticate.admin to work properly.

Hope that made it a little clearer!

On a sidenote, what is the purpose of the _index/route.tsx and who calls this route?

This is a landing / marketing page that you can customize however you want, if you want to show it to users to encourage them to try the app / other features that aren't app related. You can reach it by visiting https://<your-app-url> directly from the browser :)

zirkelc commented 1 week ago

Thanks again for the quick reply, I got it! :) I will try that out and see how it works.

I have two alternative ideas and I'd like to hear your opinion on them:


paulomarg commented 1 week ago

I think we should do Option 2, but to be transparent it might be a little while before we can get to it. If you feel comfortable changing / testing it, please feel free to contribute a PR and we'll prioritize it.

zirkelc commented 1 week ago

That would be great! Sure, I'll be happy to work on this and submit a PR in the next few days.

Thanks for your support!