Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

March fun react #1

Open
wants to merge 7 commits into
base: march_fun_base
Choose a base branch
from
Open

March fun react #1

wants to merge 7 commits into from

Conversation

ralphcallaway
Copy link
Owner

BASS training

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

@ralphcallaway
Copy link
Owner Author

ralphcallaway commented Mar 29, 2018

@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

Copy link
Collaborator

@ChuckJonas ChuckJonas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

isTouchSaving: false,
touchCount: 0,
};
this.touchOppty = this.touchOppty.bind(this);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

public async touchOppty() {
const newCount = this.state.touchCount + 1;
this.setState({ isTouchSaving: true });
const oppty = new Opportunity(); // ts-lint likes const, but we're still mutating?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

<Layout>
<Spin indicator={loadingIndicator} spinning={this.state.isOpptyLoading}>
<Layout.Header>
<h1>Opportunity Toucher: "{this.state.oppName}"</h1>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public async touchOppty() {
const newCount = this.state.touchCount + 1;
this.setState({ isTouchSaving: true });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

interface State {
isOpptyLoading: boolean;
isTouchSaving: boolean;
oppName: string;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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});

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants