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

(history): Drop states borrow before callback invocation #285

Merged
merged 1 commit into from
Jan 21, 2023

Conversation

kinnison
Copy link
Contributor

To prevent situations of 'already mutably borrowed: BorrowError' when invoking callbacks from BrowserHistory where state is in use, drop the borrow before invoking the callbacks.

This fixes #284

@kinnison
Copy link
Contributor Author

I do not believe the test failures are my fault; but I cannot seem to run tests locally (maybe nixos is a problem?) so I can't be sure.

@ranile
Copy link
Collaborator

ranile commented Jan 14, 2023

Can you add a test for this?

@kinnison
Copy link
Contributor Author

Can you add a test for this?

I can certainly try. As I noted though, the test suite as a whole will not run for me. Do you have a suggestion as to a test I could use as reference?

@ranile
Copy link
Collaborator

ranile commented Jan 15, 2023

You can look at any of the tests in the history crate as a reference.

If you can't run the tests locally (which should be easy with wasm-pack), you can just push the changes and let the CI run those tests. The history tests currently pass so if anything goes wrong, the "browser tests" workflow will fail

@kinnison
Copy link
Contributor Author

Cool, I'll do my best to sort a test out tomorrow evening; I've been a tad busy working on my app today (with this patch in place) :D

@futursolo
Copy link
Collaborator

I do not believe the test failures are my fault

CI should now pass if you merge the changes.

To prevent situations of 'already mutably borrowed: BorrowError'
when invoking callbacks from BrowserHistory where state is in
use, drop the borrow before invoking the callbacks.

Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
@kinnison
Copy link
Contributor Author

Thanks @futursolo

I managed to get wasm-pack to run history tests well enough to test the small change I added to the browser-history test to confirm that this approach is right. @hamza1311 Do you want me to cover all the state related functions or is this enough?

Copy link
Collaborator

@ranile ranile left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks.

If you want to improve the test coverage, that's always welcome. Feel free to make another PR

@ranile ranile merged commit f745e94 into rustwasm:master Jan 21, 2023
@ranile
Copy link
Collaborator

ranile commented Jan 21, 2023

Released in gloo-history 0.1.3 🎉

@kinnison
Copy link
Contributor Author

Released in gloo-history 0.1.3 tada

Fantastic, thank you

@kinnison kinnison deleted the fix-284 branch January 22, 2023 00:15
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.

gloo-history: If a callback retrieves a Location, then any _with_state function can panic.
3 participants