March fun react

[ralphcallaway]

BASS training

• basic page to increment touch field on opportunity
• basic antd styling
• all in one component
• tsforce api communication

[ralphcallaway]

@ChuckJonas if you have some time can i get a hyper critical review? wasn't sure if you needed extra perms for that so i added you as a collaborator, ignore that if it's not needed

[ChuckJonas]

@ralphcallaway looks good! Nothing sticks out that wouldn't be acceptable in a production env, but I did add a bunch of feedback and tried to include references to help take your react/typescript skills to the next step.

[ChuckJonas]

@ralphcallaway I'm sure you learned this in the react training, but you can use => functions instead of bind! It's much cleaner approach IMO. Basically all you need to do is change:

public async touchOppty() {...}

to

public touchOppty = async () => {...}

then you don't have to bind to this in the constructor

[ralphcallaway]

@ChuckJonas so i'm a bit confused, I thought that wasn't advisable since it would lead to the event handler function being re-initialized everytime the component was rendered ...

came to it with this link https://stackoverflow.com/questions/36677733/why-shouldnt-jsx-props-use-arrow-functions-or-bind in your tslint.json file

does that not apply if I do thinks like you're describing?

[ChuckJonas]

It recommends const because oppty never changes. EG: if you ran oppty = new Opportunity(); again, you would now be assigning oppty to a new location (which const wouldn't allow).

const has no impact the object properties, as these are technically really just structs (each property get it's own memory assignment).

IF you did want immutability, you have a couple options.

You could use the Readonly mapped type:

const oppty :Readonly<Opportunity> = new Opportunity();
oppty.name = 'abc' //Cannot assign to 'name' because it is a constant or a read-only property.
oppty.account.name = 'abc' //no error thrown :0

However, as you can see, it only makes the object first class properties immutable (typescript 2.8condition types now support DeepReadonly but it doesn't come packaged in ts yet).

In order to provide immutable piece of mind, ts-force generates a readonly & partial interfaces for each object: [ObjectName]Fields. So in this case, if you wanted full immutability, you can do this:

const oppty :OpportunityFields = new Opportunity();
 oppty.name = 'abc' //Cannot assign to 'name' because it is a constant or a read-only property.
 oppty.account.name = 'abc' //Cannot assign to 'name' because it is a constant or a read-only property.

If I use an SObject directly in my reducer I make sure it's typed to this "Fields" interface so I don't accidentally mutate.

Then, if you need to mutate and save, you can simply pass it back into the constructor of the corresponding class:

const mutatableOpp = new Opportunity(oppty);
mutatableOpp.name = 'asdf';
mutatableOpp.update();

[ralphcallaway]

that makes sense, thanks for the detailed explanation, that really clarified things, const just means same reference, if you need read only objects you've got some options (i also saw Object.freeze as something that could be used)

[ChuckJonas]

Best Practices would want you to break these out into stateless components. The rule of thumb is:

If it has state, it shouldn't output any HTML

antd makes this much easier to achieve because it's vastly reduces the boiler plate HTML you'll need.

In general, I think this is a very important rule to help separate presentation logic away from business logic, but in this simple case would definitely be overkill IMO.
This article on Presentational and Container Components does a great job of explaining why.

We also have a skill challenge for just this (however, it's not great IMO)

[ralphcallaway]

awesome, that rule of thumb is great, makes applying it really easy.

[ChuckJonas]

Heads up: setState is actually asynchronous. It's actually a pretty common bug people create and can cause confusion.

I doubt it would ever cause an issue here, because the latency behind the HTTP Callout & and the fact that there isn't really any impact to setting isTouchSaving: false, but there are instances were you end up overwriting state in a bad way. A simple fix would be to pass the dependent state update in as a function into the setState callback

this.setState({ isTouchSaving: true }, ()=>{
    //guaranteed to have isTouchSaving == true
});
//not guaranteed isTouchSaving == true

[ralphcallaway]

woah, okay, did not realize that. do you know if it's safe to assume that set state changes are enqueued in order?

i.e. in this case the hypothetical race condition would be, enqueue state isTouchSaving: true, make api call, enqueue state isTouchSaving: false. as long as the first state change always come through first seems like we'd be good in this scenario. in other words, as long downstream code isn't reading the state in a way that relies on the initially step being completed we could stay blissfully ignorant of this ...

[ChuckJonas]

you could just store the OpportunityFeilds directly on the state and then when you need to update state, use ES6 spread syntax. to do so without mutating:

let newOppty = {...this.state.oppty, ...{touches: newCount}};
await new Opportunity(newOppty).update(); 
this.setState({oppty: newOppty});

[ralphcallaway]

so much cool syntax

in your experience has storing the full sobject (the immutable fields instance) worked out better than storing just the data points you need? seems like it would be less to manage, but could invite extra functions you didn't intend (or maybe that's a positive since you don't need to adjust your state logic every time you want to start referring to a new field on a page)

[ChuckJonas]

@ralphcallaway that's a great question.

There's definitely a trade off to each. It's MUCH easier and efficient (productivity wise) to just store the entire object. When you are rapidly prototyping, it makes things come together really fast.

However, it violates Idiomatic redux for two reasons:

1: the SObjects returned by ts-force are classes and have functions, which apparently can have negative impacts on performance (you are only suppose to store POJOS).

2: ts-force allows you to pull dependency trees down and they are all stored on the object. This violates the best practice of normalizing your reducers. You could easily use a library like normalizr to solve this issue

My approach has been to follow this simple rule:

It's ok to store SObjects in the reducer ONLY when you don't need to mutate the objects themselves. It's probably not a huge deal to change first class properties, but definitely shouldn't change children objects

The only downside has been that my state size has gotten really big, due to duplicate relationship data being pulled down. For example, I'm storing treatments on every Spotlight Product. There might be 5000 products in my store, each with object data, but there may only be 15 distinct treatments across all of them.

I don't think this has a big impact on rendering performance, but it does make things like trying to store state in Saleforce field harder (I currently need 10 long text fields to be safe!).