Skip to content

Infra/modifiers perf improvement #2136

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

Merged
merged 3 commits into from
Jul 26, 2022
Merged

Conversation

lidord-wix
Copy link
Contributor

@lidord-wix lidord-wix commented Jul 25, 2022

Description

Update some modifiers extraction methods to improve performance.
I checked a specific session (on the private demo app, iOS) before and after these changes. the results are in ms:

before: {
  "backgroundColor": 291.95974069833755,
  "color": 1362.02895539999,
  "typography": 878.3454459905624
},
after: {
  "backgroundColor": 54.971199452877045,
  "color": 97.36799198389053,
  "typography": 85.1839787364006
},

Also, I added a function to update the modifiers' colors that should be called after loading some colors/scheme.
(and I call it on private in preset.config.ts)

Changelog

Improve modifiers' performance

@@ -100,30 +103,34 @@ export type ContainerModifiers = AlignmentModifiers &
BorderRadiusModifiers &
BackgroundColorModifier;

export function updateColorModifiers() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you'll think about it, maybe it's better that the Colors class will maintain this logic and provide a cached list of colors keys.
By moving this logic to the Colors class, you can also update the cached list whenever loadColors is being called - so you don't need to worry about it as a consumer.
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wondered about it.. you can see my note in this PR description: https://github.com/wix-private/wix-react-native-ui-lib/pull/2473

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh ok, I didn't see the private PR yet.
I think we should call and keep it in the Colors class, what are the cons for doing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only disadvantage I can think of is that it'll cause an overhead in the loadColors method in a case that the user calls it many times in his app.. but I guess it's a rare case and we can ignore it
I'll update it :)

@lidord-wix lidord-wix requested a review from ethanshar July 26, 2022 08:48
@ethanshar ethanshar merged commit b09e01b into master Jul 26, 2022
@lidord-wix lidord-wix deleted the infra/modifiers_perf_improvement branch August 4, 2022 09:15
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