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

ApplicationProvider shouldn't process mappings everytime it's mounted #613

Closed
1 task
DaniShalash opened this issue Sep 14, 2019 · 15 comments · Fixed by #895
Closed
1 task

ApplicationProvider shouldn't process mappings everytime it's mounted #613

DaniShalash opened this issue Sep 14, 2019 · 15 comments · Fixed by #895
Labels
📱 Components components module-specific

Comments

@DaniShalash
Copy link

DaniShalash commented Sep 14, 2019

I'm submitting a ...

  • bug report
  • [ x] feature request

Issue description

This is not an issue with UI Kitten itself, but rather a request to make play well with a very popular library (react-native-navigation by Wix) and maybe others.

Current behavior:
In order to use the framework with its theming capabilities we should wrap our app with UI Kitten ApplicationProvider.
ApplicationProvider uses Eva System to process mapping schemas upon its construction, which takes a couple of seconds on an average Android phone (Which is fine).

Now, In case we're using react-navigation as a navigation solution, ApplicationProvider will be mounted only once at startup (process mapping once), and then all the navigation will take place inside that provider node, and there are no issues in this case.

however, if react-native-navigation is being used, and as you may know, it works by registering each screen component individually and in isolation of others after the app is launched, we have to wrap each screen component with whatever providers being used, including redux, and in this case, UI Kitten ApplicationProvider, thus, whenever a page is being pushed or a modal being shown, the Application provider will process the mapping again which is causing a few seconds delay before the screen component being mounted.

Expected behavior:
Unless mapping prop has changed, Application provider should use a pre-processed data from previous component construction.

Steps to reproduce:
It can be simply reproduced by using react-native-navigation with UI Kitten.

Other information:

OS, device, package version

React-native 0.60
Platforms: Android, iOS
UI Kitten Version: 4.1.0
react-native-navigation: 3.0.0
@artyorsh
Copy link
Collaborator

Hi. Big thanks for reporting this as we were not able to test it with react-native-navigation. Unfortunately, we're able to process mappings just because we had an idea to support custom mappings. Btw, can you please share related code so I can play with it? AFAIK, react-native-navigation forces you to make its navigator the root component. I guess, we can get rid of this simply moving ApplicationProvider to the top

@kabus202
Copy link

kabus202 commented Dec 6, 2019

this issue still existing, did you guys find a solution already? @DaniShalash @artyorsh

@artyorsh
Copy link
Collaborator

artyorsh commented Dec 6, 2019

@kabus202 we can't focus on this at the moment. This issue is also mentioned in our docs

@DaniShalash
Copy link
Author

DaniShalash commented Dec 6, 2019

@kabus202
I did overcome this issue but it's kind of a work around, anyway, here's the summary:

The problem:

When using react-native-navigation by Wix, every time you push a screen/modal/view in your app, UI Kitten's Application provider is re-processing the styles mappings, which is the process that's causing the performance issue.

The Solution:

If the "ApplicationProvider" can process the style mappings once, and then cache the result for the subsequent times (or even better if we can process these results at build time and save it to a local file to be used in real time) then we can solve the issue.

So here's the solution in 2 different ways:

The quick way:

  1. Copy the code of UI Kitten's "ApplicationProvider" to a local file "CustomApplicationProvider", and add a "console.log" statement to log the output processed mappings.
  2. Copy the results from console and save to another file "processedMappings.json".
  3. Modify "CustomApplicationProvider" to use "processedMappings.json" instead of processing the mappings.
  4. Use "CustomApplicationProvider" instead of "ApplicationProvider" to wrap your components, and now whenever you push a view, ApplicationProvider will spend no time processing anything.

The better way:

  1. Based on the code from "ApplicationProvider", create a js file that processes the style mappings and dumps them in a file "processedMappings.json".
  2. Create a npm postinstall job that runs that js file.
  3. Copy "ApplicationProvider" code to a local file "CustomApplicationProvider", and modify it to use "processedMappings.json" instead of processing the mappings.
  4. Use "CustomApplicationProvider" instead of "ApplicationProvider" to wrap your components, and now whenever you push a view, ApplicationProvider will spend no time processing anything.

### Notes:

  • Consider your custom mappings in the process (if you have any).
  • This is not a permanent solution, and you have to review it with every new version of UI Kitten.

@artyorsh
Sorry I didn't reply earlier, looks like I forgot about it :/
Do you still need any help?

@kabus202
Copy link

kabus202 commented Dec 7, 2019

@DaniShalash thank you for you explanation, i manage to get it working!

@artyorsh
Copy link
Collaborator

artyorsh commented Dec 7, 2019

@DaniShalash thanks for the detailed answer. We need to support mapping customization, hence we do it in runtime. I've already thought about parsing mapping.json provided by Eva during the build time but this requires final users having something like eva.config.js (just like with the case of babel or metro) so we can watch for this file to do the ApplicationProvider job.

I would like to discuss this if you have any other ideas on doing this better

@kabus202 this will work, but keep in mind that you need to do this manually every time you update @eva-design/eva package (in case you follow the quick way)

@DaniShalash
Copy link
Author

@artyorsh
I'm not sure about this, but I guess since in most cases the Mappings are not subject to change in runtime, so maybe we can cache the processed output, and thus the process delay will occur only once, and that's when the app launches (which is fine I guess).
Now in case the user wants to change the mappings in runtime, we can provide methods for that, I can imagine it like the way react-native-elements provide updateTheme and replaceTheme methods to their theme consumers

In other words, the themeProvider can be a hoc that accepts the app component along with style mappings and theme as args, and after processing and caching the output it will provide changeMappings and changeTheme props to appComponent.

I know this is kind of complicated in comparison to what we have now, but it'll make UIKitten navigation-solution-agnostic, which I guess is pretty much important.

BTW, I still find something like eva.config.js more convenient, it'll limit changing Mappings in runtime, but it'll cut the processing delay anywhere, even in app startup.

@sudomann
Copy link

Probably similar to #593:

I was curious whether Expo Constants package dynamically updates Constants.statusBarHeight, and so I went into android display settings and changed display size (which affects the scale of all UI items of course). The other apps on the device resized just fine except mine which is built with ui-kitten. The window flashed when I tapped it from the recent apps list, as if it was going to bring back the app then it just closed. When this happens, you have to go and open it again and start over what you were doing. It's like you swiped the app away yourself to close it or discard from memory.

@artyorsh
Copy link
Collaborator

artyorsh commented Feb 14, 2020

@DaniShalash Could you please review the idea of this pr and give feedback?

@artyorsh
Copy link
Collaborator

Resolved in v4.4.1 🎉

@qlereboursBS
Copy link

qlereboursBS commented Mar 20, 2020

Hi @artyorsh I installed the v4.4.1 yesterday and it didn't fix my issue #855
However, I used the comment from @DaniShalash to create a workaround. After testing with this solution, it reduced the loading time from 15 seconds to 3 seconds.

  • First, create a custom processor (based on the Application provider found in rn-ui-kitten package):
require("@babel/register"); // This will be needed to execute code with imports in a nodejs environment
const mapping = require('@eva-design/eva').mapping; // this is the mapping I pass to <ApplicationProvider> in my App.js
const SchemaProcessor = require('@eva-design/processor').SchemaProcessor;
const merge = require('lodash.merge');
const customMapping = require('./custom-mapping'); // Change this to target your custom mapping file
const fs = require('fs');

const process = function() {
  const schemaProcessor = new SchemaProcessor();
  const customizedMapping = merge({}, mapping, customMapping);
  const processed = schemaProcessor.process(customizedMapping);
  fs.writeFileSync('./src/theme/custom-mapping-processed.json', JSON.stringify(processed)); // change this to where you want to write the processed file
  console.log('Custom mapping processed written in file !');
};
process();
  • Then edit rn-ui-kitten application provider in node_modules/react-native-ui-kitten/theme/application/applicationProvider.component.js. Change those lines:
        this.state = {
            styles: this.props.styles,
        };
        // this.schemaProcessor = new processor_1.SchemaProcessor(); // we don't need shemaProcessor anymore
        this.createStyles = (mapping, custom) => {
            const customizedMapping = lodash_merge_1.default({}, mapping, custom);
            return this.schemaProcessor.process(customizedMapping);
        };
        if (!this.state.styles) {
            const { mapping, customMapping: customMappingProcessed } = this.props; // I renamed the variable bc the customMapping received will already be processed
            // this.state.styles = this.createStyles(mapping, customMapping); // we don't need to call the method anymore
            this.state.styles = customMappingProcessed; // directly set the styles to the processed mapping
        }
  • Create the processed file with node <yourFile>

  • After changing the node_module, patch the package to make it persistent after any install, you can use patch package

  • Edit your call to ApplicationProvider:

// App.js
import processedMapping from './src/theme/custom-mapping-processed.json';
...


return (
    <ApplicationProvider
      mapping={mapping}
      customMapping={customMappingProcessed}
      theme={theme}
    >
        ...
    </ApplicationProvider>
)
  • Don't forget to automate the creation of the processed file (before you create a release, or when you start the app), you just have to call the nodejs script

@artyorsh
Copy link
Collaborator

@qlereboursBS this and related issues (e.g #855) were closed because now we have a package that processes mapping during the build time. Read the docs

@qlereboursBS
Copy link

Thanks, I didn't see that your message was linking to a changelog.

@DaniShalash
Copy link
Author

Hey guys, sorry I’ve quiet busy lately so I couldn’t have time to respond, however I’ll be free soon.

Just wanted to point out that the current solution is much better, but honestly I’m still holding to 4.2.x, the reason:

The processed mapping output in 4.2.x is about 35k lines json file, while in the latest version it’s more than 100k!
And the more components added, the bigger this file is getting and more delay it produces, taking into consideration that most of the new components (like datePicker) doesn’t work with RNN, and its styles are just an unnecessary heavy load.

Also, if we take a look at that json file, there’s a lot of unnecessary duplications, like for example, all of the style variants of a single component share 90% of their style attribute values.
Creating a base style that’s always applied to the component, then adding variants styles....

This would result in a much more efficient and compressed file than what we have now.

I know all I did now is talking, but I just wanted to express my opinion for the time being now.

@artyorsh
Copy link
Collaborator

@DaniShalash thanks for sharing! Can you please give more details on it?

most of the new components (like datePicker) doesn’t work with RNN

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📱 Components components module-specific
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants