-
Notifications
You must be signed in to change notification settings - Fork 167
Fix conditions where BackgroundReplacementProvider needs to be re-rendered, Fix to not log the entire options object #985
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
Conversation
76dc47a to
bd2b5f7
Compare
| return true; | ||
| } | ||
| if ( | ||
| prev?.imageBlob?.size === next?.imageBlob?.size || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to keep the comparison for imageBlob?.size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the image size comparison because now it supports dynamically change replacement image in an active processor. The processor recreation when image changes will cost to fetch assets again.
There're two independent ways to update BGR:
- dynamically change replacement image in an active processor via
changeBackgroundReplacementImagefromuseBackgroundReplacementhook - update the
optionsprops ofBackgroundReplacementProvider
Is it a breaking change that removing the comparison for imageBlob seems to affect the second flow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to remove this comparison.
- image in the constructor should only take effect when initiate the component, changing image should always through API
processor.setImageBlob(imageBlob). - Also, I verified that assets will be re-fetched if BackgroundReplacementProvider re-renders, we may want to avoid it.
I will merge this PR only after QA's signoff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a breaking change that removing the comparison for imageBlob seems to affect the second flow?
Before this change, the provider will never be re-created even if pass in a different image in constructor. Thus, I don't think it's breaking. correct me if wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- image in the constructor should only take effect when initiate the component, changing image should always through API processor.setImageBlob(imageBlob).
- Also, I verified that assets will be re-fetched if BackgroundReplacementProvider re-renders, we may want to avoid it.
I agree this is best practice. However before previous release, processor.setImageBlob was not available in React SDK. And builders can only rely on the options props change to re render entire BackgroundReplacementProvider to update the processor. Though there's a bug to make it never works, so this
change may not be a breaking one.
My thought is the behavior of BGB and BGR provider should align meaning any change of their configuration in the provider props should cause the provider to re-render.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What use case is there for the BGBlur or BGR Provider to re-render? When would they need to kill the old processor and create a new one?
I think the preferred behavior is that the BGBlur and BGR provider just take in an initial props, but don't re-render on those prop changes.
Instead, we should just expose functions to dynamically change the processor with a new image blob, or new blur strength.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, if cx changes replacement image in props, process will not be re-created. It will be a breaking change if it allows re-creation upon props change.
Thus, I think removing image.blob check keeps it non-breaking, meanwhile, it pushes cx to use processor.setImageBlob()
Issue #:
Description of changes:
BackgroundReplacementProviderneeds to be re-rendered. Remove the image size comparison because now it supports dynamically change replacement image in an active processor. The processor recreation when image changes will cost to fetch assets again.optionsprops inBackgroundReplacementProvider, the optionalloggerprop could potentially be nested JSON.Testing
Have you successfully run
npm run build:releaselocally?Yes
How did you test these changes?
Locally and private build tested by QA
If you made changes to the component library, have you provided corresponding documentation changes?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.