Skip to content

tutorial: In the onDestroy example, explicitly show the memory leak #5515

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

Merged
merged 8 commits into from
Jun 27, 2021

Conversation

fivemru
Copy link
Contributor

@fivemru fivemru commented Oct 11, 2020

In the onDestroy example, the current solution does not explicitly (visually) show the memory leak and the moment when the component was destroyed.
If I remove the onDestroy block in the utils.js file on the site, nothing visually changes. It will not be clear if the callback has been executed.
I decided to make a more illustrative example in my opinion, what do you think about it?

Demo repl

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@benmccann
Copy link
Member

This contains a superset of the code in #5513

@fivemru
Copy link
Contributor Author

fivemru commented Oct 22, 2020

This contains a superset of the code in #5513

Sorry, forgot to switch to master, did a revert of commit 159cf86.

Copy link
Member

@tanhauhau tanhauhau left a comment

Choose a reason for hiding this comment

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

The idea of visualising the memory leak is good.

probably a few suggestions over here to make it better:

  • probably update the instructions as well, to explain what we are seeing (the timer still ticking even though the component is unmounted), and why is it wrong / bad
  • actually another way of visualising the memory leak would be to have a console.log, but i think either way is fine for me.
  • probably name the inner component with a better name, such as Timer instead of Inner. try make it more friendly and real.
  • same thing goes for the counter=1, something more friendly, like the current message, such as "The component has been created since 5 seconds"

@fivemru fivemru requested a review from tanhauhau November 22, 2020 01:40
@fivemru
Copy link
Contributor Author

fivemru commented Nov 22, 2020

Demo A
Demo B

@stale
Copy link

stale bot commented Jun 26, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Jun 26, 2021
@stale stale bot removed the stale-bot label Jun 26, 2021
@pngwn pngwn added site and removed site: tutorial labels Jun 26, 2021
@benmccann benmccann merged commit bca46b9 into sveltejs:master Jun 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants