search

notrab / react-use-cart

React hook library for managing cart state
http://npm.im/react-use-cart
Apache License 2.0
367 stars 48 forks source link

feat: Add support for multiple item prices #70

Closed ynnoj closed 2 years ago

ynnoj commented 3 years ago

Fixes #67.

notrab commented 3 years ago

@ynnoj nice start!

I like the direction this is going, but this would indeed introduce a more verbose API for adding items (something I glossed over in #67 - and doesn't particularly bother me). It's now tied to the concept of currencies, whereas before it wasn't.

Perhaps your idea of conditional types is worth exploring, and would be backwards compatible. price would either be an integer or array... What do you think?

Happy to jump on a call to pair through this if you wanted.

ynnoj commented 3 years ago

It's now tied to the concept of currencies, whereas before it wasn't.

I'm not sure this is actually an issue though seeing as though most (all?) payment processors will require currency info anyway. Seems logical to store that data in context of the cart.

I don't really have the TS expertise (and time) to explore the conditional type approach, so if that's the desired direction I will have to shelve this PR temporarily.

ynnoj commented 3 years ago

@notrab Been thinking some more about this, particularly the conditional types and the impact that would have on the cart calculations and response.

Would anything prevent me adding multiple items with different price types? For example:

[{
  id: '1',
  price: 200,
  quantity: 2
}, {
  id: '2',
  price: [{
    currency: 'GBP',
    value: 100
  }, {
    currency: 'EUR',
    value: 150
  }]
}]

This would impact the cartTotal calculations as you'd end up with a total without a currency designation, which would seem particularly confusing. Something like:

[{
  currency: 'GBP',
  value: 100
}, {
  currency: 'EUR',
  value: 150
}, {
  value: 400
}]

Another idea I've thought about is setting a base currency at cart level, then you can transform the standard price: number accordingly.

notrab commented 3 years ago

@ynnoj you raise a good point on that. Let me think about it a bit more as well.

ynnoj commented 3 years ago

@notrab Any further thoughts on this?

notrab commented 2 years ago

I'll go ahead and close this for now as it's a bit stale and outdated but certainly something that someone could do more research on how we support this with a clean interface if there's enough interest. Thanks for all your hard work on this @ynnoj