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

Ensure PortalTarget onChange is called on initial render #537

Merged
merged 2 commits into from
Jul 1, 2022

Conversation

meirish
Copy link
Contributor

@meirish meirish commented Jun 24, 2022

Fixes #527.

@meirish
Copy link
Contributor Author

meirish commented Jun 24, 2022

I looked into this a bit, but wasn't sure how you'd prefer to fix it.

The issue seems to be that because of the next wrapping here https://github.com/kaliber5/ember-stargate/blob/2f55bd96bccea7e3414c33b2ec92526cb2738a70/packages/ember-stargate/src/services/-portal.ts#L48 when registering the PortalTarget, that means that target here: https://github.com/kaliber5/ember-stargate/blob/2f55bd96bccea7e3414c33b2ec92526cb2738a70/packages/ember-stargate/src/services/-portal.ts#L65 is undefined so the onChange doesn't get called.

It looks like wrapping the body of registerPortal here in a next like registerTarget does fixes this, but I'm uncertain if that has any knock-on effects.

@simonihmig
Copy link
Owner

Thanks for the test! There are some linting errors, could you please fix them? (you can run yarn lint locally)

Your observations regarding next() make a lot of sense. I don't really recall why I used that in the first place. Will look into it...

@simonihmig
Copy link
Owner

Ok, so without that next() call, this test is failing, with Portal target with name main already exists. Which makes sense, as when creating a new target with the same name, it would be in conflict with the "soon to be destroyed but not quite yet" target, when the order of calls is 1. register new target, 2. unregister old target. So we have to make sure that unregister happens first, which is what the next() call on register (not unregister!) does.

So it would be consistent to also wrap registerPortal in next(), to ensure it happens always after any unregisterPortal calls, when both are called in the same event loop. Which is what you suggested.

So @meirish, if you have the time to turn this "failing test" PR into a full bug fixing PR, by adding a commit that does what we discussed above, then please do so, would be highly appreciated! 😀

but I'm uncertain if that has any knock-on effects.

I don't think so. If all other tests still pass, which is what I would assume, then I am confident this is ok!

@meirish meirish force-pushed the b-initial-onchange branch from 1c1b969 to 04d807e Compare June 30, 2022 21:23
@meirish meirish changed the title Add failing test for PortalTarget onChange on initial render Ensure PortalTarget onChange is called on initial render Jun 30, 2022
Copy link
Owner

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

All tests pass! 🎉

Thank you!

@simonihmig simonihmig added bug Something isn't working and removed internal labels Jul 1, 2022
@simonihmig simonihmig merged commit 277abef into simonihmig:master Jul 1, 2022
@simonihmig
Copy link
Owner

@meirish meirish deleted the b-initial-onchange branch July 1, 2022 15:22
@meirish
Copy link
Contributor Author

meirish commented Jul 1, 2022

Awesome, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

onChange doesn't fire on initial render
2 participants