Skip to content

Fixed bug in dependency deletion.#2421

Open
MaximumCookie wants to merge 1 commit intojonataslaw:masterfrom
MaximumCookie:master
Open

Fixed bug in dependency deletion.#2421
MaximumCookie wants to merge 1 commit intojonataslaw:masterfrom
MaximumCookie:master

Conversation

@MaximumCookie
Copy link
Copy Markdown

Before the Binding creates dependencies for a new route, the dependencies from the old route are marked as "dirty". However, if the new route adds one of those same dependencies, then the dirty dependency is moved to the 'lateRemove' property and the root dependency is no longer marked 'isDirty'. This means, when Get.delete() is eventually called on the dependency, the lifecycle 'delete()' is called on the new dependency instead of the old one, and the old dependency ends up being removed from memory (To summarize: The delete() method is called on the wrong controller if both the old and new route have the same controller in their Binding.).

It seems like the delete() lifecycle method should always be called on the same dependency that ends up being removed from memory, which this change fixes.

Every PR must update the corresponding documentation in the code, and also the readme in english with the following changes.

Pre-launch Checklist

  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or @jonataslaw said the PR is test-exempt.
  • All existing and new tests are passing.

Before the Binding creates dependencies for a new route, the dependencies from the old route are marked as "dirty".  However, if the new route adds one of those same dependencies, then the dirty dependency is moved to the 'lateRemove' property and the root dependency is no longer marked 'isDirty'.  This means, when Get.delete() is eventually called on the dependency, the newly created dependency gets deleted instead of the old one.  However, before that delete occurs, the new dependency is already in use by the new page which is likely to cause problems.
@jonataslaw
Copy link
Copy Markdown
Owner

Wow, thanks for that. Looks like you're right.
As this is critical, I'll write some tests and see if it doesn't cause any side effects and then I merge it.

@CeAlong
Copy link
Copy Markdown

CeAlong commented Aug 14, 2023

I would like to know when this merge will be released.

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.

4 participants