loadsmart / rn-salesforce-chat

React Native wrapper for Salesforce SDK chat
MIT License
7 stars 10 forks source link

ADR 1: Define how to better pass parameters to native side #5

Closed lenoirzamboni closed 4 years ago

lenoirzamboni commented 4 years ago

Architecture Decision Review - ADR

We need to discuss a better way to pass parameters to the native side while using the API methods. The current way is not so didactic, and this can cause confusion and even errors while using the library.

We have two options:

  1. Modify the current native methods to accept a dictionary as param;

Pros:

Cons:

Example of this idea:

import { NativeModules } from 'react-native'

... 

const salesforceChat = NativeModules.RNSalesforceChat
salesforceChat.createPreChatData({
  agentLabel: "Account ID",
  value: "b1174ff9-7b5b-4c2a",
  isDisplayedToAgent: false,
}))

 

  1. Create and export methods on our index.js and future index.d.ts. These methods would accept objects defined by interfaces that we can also create and export to the user.

Pros:

Cons:

Example of this idea:

// PreChatData.ts

export interface PreChatData {
  agentLabel: string
  value: string | undefined
  isDisplayedToAgent: boolean
}
import { createPreChatData } from '@loadsmart/rn-salesforce-chat'

...

createPreChatData({
  agentLabel: "Account ID",
  value: "b1174ff9-7b5b-4c2a",
  isDisplayedToAgent: false,
})
lenoirzamboni commented 4 years ago

@barbosa @pedrohperalta can you take a look at this? Feel free to comment on it and bring other ideas and suggestions.

barbosa commented 4 years ago

@lenoirzamboni good ADR 🙂

Which of the two options are you more inclined to use? And why?

lenoirzamboni commented 4 years ago

@lenoirzamboni good ADR 🙂

Which of the two options are you more inclined to use? And why?

@barbosa I'm inclined to use the second option. I believe it's a more common approach, easier to understand and use.

barbosa commented 4 years ago

Great!

I also like option 2.

Option 1 works fine but I believe it exposes to much the internal implementation. Although we're not limited by that, I'd prefer option 2's ability to have clear and well-defined methods/API.

The con mentioned in option 2 doesn't worry me that much since I think it's implicit that every time we change something on the native side, we'd need to make sure the external contract won't break.

lenoirzamboni commented 4 years ago

@barbosa All right! I'll work on these changes then!