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

Render components only after we have collected the Ion props #482

Merged
merged 10 commits into from
Sep 15, 2020

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Sep 12, 2020

This PR is an attempt to solve the problem of all connected React components rendering without any initial data from the Ion. It achieves this without the addition of a memory cache and all other Ion.connect() usages and subscribers will remain untouched.

As far as I can tell, there seem to be no performance impact here and there is not much difference between first getting this data from storage and loading it with a single call to render vs. mounting the component and then rendering many times as data is sent back to each connection.

The main change is that a component connected withIon will take the Ion props along with it for the first render. I think this is a really important change to make early on because:

  1. Not doing this will cause hacks around componentDidUpdate() to replace prop handling logic done in componentDidMount() or the constructor()
  2. We can stop worrying about what props exist or don't exist in components for the first lifecycle method. If the component renders with a null prop for any value then there is no key for that value. It won't first render with undefined then appear to be null so we can get more value out of things like propTypes and write less confusing code.

cc @tgolen and cc @chiragsalian who just ran into an issue related to initial props being unknown

Curious to hear everyone's thoughts on this. This is my best attempt at staying respectful of the Ion philosophy while also keeping React best practices in mind.

@marcaaron marcaaron self-assigned this Sep 12, 2020
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

I really like the core idea for this, and it was actually just something that had run through my mind when I was thinking about it with Chirag last week. I think it's a good change that will make working with withIon easier for engineers new to the codebase.

src/components/withIon.js Outdated Show resolved Hide resolved
src/lib/Ion.js Outdated Show resolved Hide resolved
src/components/withIon.js Outdated Show resolved Hide resolved
@marcaaron marcaaron changed the title [WIP] Render components only after we have collected the Ion props Render components only after we have collected the Ion props Sep 14, 2020
@marcaaron
Copy link
Contributor Author

Took another crack at this one.

@marcaaron marcaaron marked this pull request as ready for review September 14, 2020 22:09
@chiragsalian
Copy link
Contributor

Damn, it works so well in my initial testing ✨
It also just does a component render once instead of twice which is so great for our performance.

Reviewing and testing some more.

tgolen
tgolen previously approved these changes Sep 14, 2020
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

This looks really great!

Copy link
Contributor

@chiragsalian chiragsalian left a comment

Choose a reason for hiding this comment

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

Left one comment to get your thoughts. Rest looks awesome and works great. Am hype 🙂

// since keys that are configured to not init with stored values will
// never appear on state when the component mounts - only after they update
// organically.
const requiredKeysForInit = _.chain(mapIonToState)
Copy link
Contributor

@chiragsalian chiragsalian Sep 14, 2020

Choose a reason for hiding this comment

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

This logic is a bit confusing with .reduce and prev and curr. If i understand correctly we just want all keys except where initWithStoredValues is defined as false.

Then could this logic be used since it looks easier,

const requiredKeysForInit = _.chain(mapIonToState)
    .omit(value => value.initWithStoredValues === false)
    .keys()
    .value();

Copy link
Contributor

Choose a reason for hiding this comment

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

Tested the same,
image

Notice i just got key "myPersonalDetails" and "network" but didn't get "testSomeKey" since it had initWithStoredValues as false.

@marcaaron
Copy link
Contributor Author

Nice tips @chiragsalian will update with that change!

Copy link
Contributor

@chiragsalian chiragsalian left a comment

Choose a reason for hiding this comment

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

LGTM

@tgolen tgolen merged commit e71f22a into master Sep 15, 2020
@tgolen tgolen deleted the marcaaron-propsOnFirstRender branch September 15, 2020 22:55
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.

3 participants