Skip to content

New architecture support#37

Open
prajnab wants to merge 11 commits intooblador:masterfrom
prajnab:master
Open

New architecture support#37
prajnab wants to merge 11 commits intooblador:masterfrom
prajnab:master

Conversation

@prajnab
Copy link

@prajnab prajnab commented Sep 30, 2022

Added support for the fabric architecture along with backward compatibility for the old architecture.

Simulator.Screen.Recording.-.iPhone.13.-.2022-10-25.at.18.33.31.mp4

@prajnab
Copy link
Author

prajnab commented Sep 30, 2022

resolves #35

@oblador
Copy link
Owner

oblador commented Oct 5, 2022

Hi, thank you for this PR! I've only run-tested this PR on iOS so far and didn't dive into the code much but I've found two major issues;

  • In Fabric on iOS, initial props and changed props are ignored, it seems it only uses the default props at all times.
  • In Fabric on iOS the app crashes when unmounting the component or reloading the app.

@oblador
Copy link
Owner

oblador commented Oct 5, 2022

Bumped the example app to latest stable which should help you debug this issue

"version": "0.6.0",
"version": "0.7.0",
"description": "Simple shimmering effect in React Native",
"main": "index.js",
Copy link
Owner

Choose a reason for hiding this comment

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

Don't forget to delete the old index file too 👍

@prajnab
Copy link
Author

prajnab commented Oct 6, 2022

Hi, thank you for this PR! I've only run-tested this PR on iOS so far and didn't dive into the code much but I've found two major issues;

  • In Fabric on iOS, initial props and changed props are ignored, it seems it only uses the default props at all times.
  • In Fabric on iOS the app crashes when unmounting the component or reloading the app.

The example is bumped to latest RN version but fabric was not enabled. Let me add two separate examples for the new and old architecture. And let me check again regarding the issues mentioned

@oblador
Copy link
Owner

oblador commented Oct 6, 2022

Enabling/disabling fabric is pretty trivial, i prefer having just one example project as it is less maintenance

Fixed crash when animating switch is toggled

Removed index.js
@prajnab prajnab requested a review from oblador October 6, 2022 11:31
@prajnab
Copy link
Author

prajnab commented Oct 14, 2022

@oblador The issue has been fixed. Please review the updated PR

@oblador
Copy link
Owner

oblador commented Oct 14, 2022

Thanks for the update, however the app is still crashing if I reload/hot refresh or toggle the animation enabled switch.

@oblador
Copy link
Owner

oblador commented Oct 28, 2022

Example project crashes on start for me 🤔
Screenshot 2022-10-28 at 15 28 05

@prajnab
Copy link
Author

prajnab commented Oct 30, 2022

Example project crashes on start for me 🤔

@oblador Can you please check if you have the latest pull and have deleted the node_modules and build folders before running the app? It looks like the error is due using property "direction" instead of "shimmeringDirection".

In the last commit(d9aef78) I had updated two properties(direction->shimmeringDirection and opacity->shimmeringOpacity) for the old architecture apps as the new ones were accepting shimmeringDirection and shimmeringOpacity.

@oblador
Copy link
Owner

oblador commented Oct 31, 2022

Yep, can get it to work now thanks! 👍 Some new/other bugs I found:

  • On Android using Fabric the Loading text is not visible.
  • On iOS using Fabric the animation duration on the Logo seems to be wrong (it's veery slow)

Screenshot_1667212632

@prajnab
Copy link
Author

prajnab commented Nov 2, 2022

@oblador Had a look at these issues and here are my observations:

- On Android using Fabric the Loading text is not visible.
Had seen this during development, but I confused it was shimmer-android issue due to this previous issue. I was not able to identify what exactly is the reason for the text component to disappear as the image component is working fine.

Also, I noticed that if I comment the text, save it and then uncomment and save it(which makes it render twice), the "Loading..." label will be visible.

- On iOS using Fabric the animation duration on the Logo seems to be wrong (it's very slow)
The duration is as expected when we first run the app. But it slows down on reload and will work fine only on relaunch. This could be due to the debug mode.

@oblador
Copy link
Owner

oblador commented Nov 2, 2022

Hmm, the linked issue was already fixed by bumping the native dependency. The iOS issue i think is related to how the prop is updated (or not updated). That prop is applied and calculated in a special way on iOS where the order seems to matter.

@prajnab
Copy link
Author

prajnab commented Nov 2, 2022

Regarding android, yes I realised it when i rechecked it after you reported. And it is working fine even in my PR for the old architecture. But I am not able to identify what exactly is the issue with new architecture.

Regarding iOS, yes i did see and realise that. Hence i am updating the duration at the end(file - on load line 44 and on update line 111.

Updated RN version of example app
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