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

SWC-7102, SWC-7131 - Fixes for ReactComponent widgets #5551

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

nickgros
Copy link
Contributor

No description provided.

@@ -14,8 +14,6 @@ public interface SynapseJSNIUtils {

public void highlightCodeBlocks();

void loadSummaryDetailsShim();
Copy link
Contributor Author

@nickgros nickgros Oct 17, 2024

Choose a reason for hiding this comment

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

details-shim was removed in #5346

I missed these references to the module, so removing them here will make an error message in the console go away. This does not fix SWC-7102

@nickgros nickgros marked this pull request as draft October 21, 2024 16:12
Comment on lines +72 to +75
public void render(ReactElement<?, ?> reactElement) {
this.reactElement = reactElement;
createRootAndRender();
}
Copy link
Contributor Author

@nickgros nickgros Oct 25, 2024

Choose a reason for hiding this comment

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

a bit of refactoring to unify the calls to Timer.run/Timer.schedule (setTimeout) and make it more clear why it's being scheduled. No logical change

Comment on lines +95 to +97
if (root != null) {
root.render(React.createElement(React.Fragment));
}
Copy link
Contributor Author

@nickgros nickgros Oct 25, 2024

Choose a reason for hiding this comment

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

onUnload was inappropriate here. This widget can only contain a React component tree (no widget children), so it's much safer to just render a fragment. This prevents bugs where the root was unmounted and destroyed when it didn't need to be.

@@ -92,11 +92,40 @@ private void createRoot() {
}
}

/**
* Asynchronously (in the task queue, via setTimeout) unmounts the root and sets it to null.
*/
private void destroyRoot() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just as in ReactComponent (v1), move the asynchronous logic inside of destroyRoot

Comment on lines +214 to +219
if (shouldDestroyRoot) {
destroyRoot();
}

// Create a fresh ReactElement tree and render it
componentToRender.createRoot();
componentToRender.root.render(componentToRender.createReactElement());
}
};
t.schedule(0);
// Schedule rendering
createRootAndRender();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that even through the timers were changed so destroyRoot and createRootAndRender now trigger two timers, they end up on the same queue, so they will execute in order.

@@ -216,20 +234,10 @@ protected void onLoad() {

@Override
protected void onUnload() {
super.onUnload();
Copy link
Contributor Author

@nickgros nickgros Oct 25, 2024

Choose a reason for hiding this comment

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

The super method is an empty implementation, so this call is not necessary

@nickgros nickgros changed the title SWC-7102 SWC-7102, SWC-7131 - Fixes for ReactComponent widgets Oct 25, 2024
@nickgros nickgros marked this pull request as ready for review October 25, 2024 16:05
@nickgros nickgros merged commit f22a87e into Sage-Bionetworks:develop Oct 25, 2024
3 checks passed
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