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

Check parcel status before calling unmount #95

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

af-tomwilkins
Copy link

@af-tomwilkins af-tomwilkins commented Mar 3, 2022

I have been running into the following error using the Parcel component:
image

This occurs when the parent application unmounts. Looking at the lib, there's no check on the parcel status before calling unmount.

My change is the equivalent of this line in single-spa-react:
https://github.com/single-spa/single-spa-react/blob/main/src/parcel.js#L66

Ensuring that unmount is only ever called on parcels with a status of MOUNTED.

I would be grateful if someone more familiar with the project could please review this, hoping it's suitable to merge!

@J1mmyC01
Copy link

J1mmyC01 commented Mar 3, 2022

Would be great to get this PR reviewed. Many thanks.

@af-tomwilkins af-tomwilkins changed the title Exit addThingToDo when actioning unmount on an unmounted parcel Check parcel status before calling unmount Mar 3, 2022
@joeldenning
Copy link
Member

I pulled down your code to add tests to it, but was unable to reproduce the problem in tests. Could you please provide a demonstration of the repo? Before merging this PR, we need to understand the root cause and have tests for it.

@af-tomwilkins
Copy link
Author

@joeldenning Thanks for looking at this. The component using the parcels is in a private repository on Bitbucket and uses an in-house parcel implementation we created to tide us over, but I've published a version using single-spa-vue to GitHub and invited you to collaborate.

Instructions to see the problem are on the README of the repository.

@joeldenning
Copy link
Member

I don't know which repository you invited me to view. Adding a test to the single-spa-vue repository would be the best approach for demonstrating that there was a problem and that there no longer is.

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.

3 participants