-
-
Notifications
You must be signed in to change notification settings - Fork 916
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
Fix #1184 #1189
base: main
Are you sure you want to change the base?
Fix #1184 #1189
Conversation
G'day team, I'm keen to get a code review on this. I reckon it's something that ought to be sorted by the single-spa library, maybe right here or over in single-spa-react/parcel. I've set up a repo showing the fix in action, took a leaf out of the book from this pattern to make the change. Just holler if there's anything else needed on my end. |
@joeldenning, what's your take on this solution? |
@MilanKovacic , what's your take on this solution? |
Hi, thank you for submitting the pull request. I will take a look. |
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.
The code change looks reasonable, I've left a couple comments.
Are you someone who I've asked not to contact me @BeastlyUnsold? I ask because issue #1184 was created by an Adobe employee, and I have been harassed by a dev at adobe who works on single-spa stuff. Your Github account was created recently and only has worked on this Adobe issue, which is enough for me to wonder whether I'm being alted by someone I've asked to never contact me again. I am cautious about it because he hasn't been able to stop interacting with me despite repeated requests and even legal action.
parcelConfig.unmount = () => { | ||
parcelConfig.unmountCalls++; | ||
return new Promise((r) => { | ||
setTimeout(r, 2000); |
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.
Why wait 2 seconds to resolve the promise?
}); | ||
// In some cases it is possible for a parcel to be in an unmounting status when `unmountThisParcel` is called a second time. | ||
// https://github.com/single-spa/single-spa/issues/1184 | ||
if (parcel.status === UNMOUNTING && !!parcel.internalUnmountPromise) { |
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.
Truthiness check is good enough for this if statement, no?
if (parcel.status === UNMOUNTING && !!parcel.internalUnmountPromise) { | |
if (parcel.status === UNMOUNTING && parcel.internalUnmountPromise) { |
}) | ||
.then((value) => { | ||
resolveUnmount(value); | ||
return value; |
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.
Deleting the internalMountPromise might be helpful for garbage collection.
return value; | |
delete this.internalMountPromise; | |
return value; |
Fix #1184
Proof of issue: https://codesandbox.io/p/sandbox/vigilant-jepsen-gkznvg