vercel / commerce

Next.js Commerce
https://demo.vercel.store
MIT License
11.33k stars 4.18k forks source link

API Limitations may Render this Unusable in Production #1045

Closed osseonews closed 1 year ago

osseonews commented 1 year ago

Been following the code extensively on this repo and spent a really long time on V1 of this repo. The basic flaw, in my opinion, of the current V2 implementation is that you are doing everything on the server for basic e-commerce operations. The problem is that every service has API limitations, which generally are calculated based on a request origin. When you hit an API on a client, the request origin is obviously different for each user, so if say your API limit is 60 per client, you will be fine (unless a specific user abuses the service). However, if you hit the API on the server, the request origin will be the same for every single user, which means that for something like the cart, you will quickly hit API limits on many production stores, as the limit will be shared by ALL users (the request origin will be the same for everyone, as the API is hit from the server). I hope the above this makes sense. Honestly, something like the cart should be done on the client, with a library like useSwr (as per V1), and other e-commerce operations can be done on the server (with new App folder). The notion of All or Nothing will likely fail in production, especially for Cart. There will be alot of bugs. I don't see why doing the cart on the client is not the default, as V1 works very well, except for a few slight issues with race conditions, which can be fixed. I think the new App directory is great, but it has specific use cases.

tmb5cg commented 1 year ago

Thanks for pointing this out. Does V1 still work if I dig through the commit history? I'd prefer if cart were kept client side

osseonews commented 1 year ago

Yes, V1 still works fine and is proven in production settings. Will it always work? Who knows, but I just don't see Vercel getting rid of the Page router anytime soon, if ever, simply because the amount of apps built on Page far exceeds App, and may for a long time. In V1 the cart is done on the client with useSwr library, and so I think the scalability is much, much better. Personally, I don't see what the benefit of App router is for things like cart (app is useful for other e-commerce features, though), and so I don't see why they refactored the cart, other than prove that App folder works.

dannyyu92 commented 1 year ago

Are you sure about this? Seems very broken if it's so. However, in the docs I found that the cart mutations from the Storefront API have no rate limiting

https://shopify.dev/docs/api/usage/rate-limits https://shopify.dev/docs/api/storefront/2022-04/mutations/cartLinesAdd

so it should be okay right? Can any devs speak to this?

osseonews commented 1 year ago

@dannyyu92 Sorry, I am not familiar with shopify API rate limitations. But, I know every other API I've used in e-commerce has rate limits based on the client. I was speaking about the issue, in general. Maybe this won't apply to Shopify, I don't know. And, in general, one of the flaws with RSC is precisely this: API rate limits, because the client is the same across all requests. Developers of these "demo" repos don't take this into consideration, b/c they are not working in real production setting with many users.

In terms of Shopify, they specifically abandoned RSC (react server components) completely in favor of Remix, as they determined that RSC was not viable in production for Shopify. So Shopify itself won't use RSC, but Vercel is recommending RSC for Shopify? It just doesn't make sense. What do Vercel devs know about Shopify that Shopify's own devs don't know? I'd be interested in seeing someone run this NextJS repo in actual production in a high traffic store.

Anyway, I don't see any benefit whatsoever of doing Cart queries/mutations on the server vs client. Doing at the client level (like in V1) will be faster and more scaleable, though there are issues with client, as well. There are certainly many benefts of using RSC for product pages, and the e-commerce content in general, but for user specific actions, like cart, order history (not even developed yet here), I don't see what the benefit is at all. Of course there are flaws in each approach, so I guess you have to determine which tradeoff you are willing to make.

tobbbe commented 1 year ago

Although I agree with @osseonews thinking, the BuyerIp header is what Shopify tells us to use to handle rate limits if using server requests: https://shopify.dev/docs/api/usage/authentication#optional-ip-header I think we should implement that header into this repo!

manovotny commented 1 year ago

Appreciate you bringing this conversation up, @osseonews.

As with many decisions in development, there is usually no universally "right" or "wrong" answer. As engineers, we assess the needs of the application, the constraints in which we need to work within, and pick the best path forward after we evaluate, experiment, and weight the pros and cons of any scenario.

As you mention, some APIs do have rate limits, which would be an important factor to consider in designing your application. In this particular use case, we use Shopify, and more specifically the Storefront API, which does not have a rate limit. Because of this, it makes doing the API communication on the server for this particular template / starter kit a good choice. A different provider with different constraints may yield a different decision or implementation.

We're happy to revisit this implementation should things change.