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

Add GC tests #176

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add GC tests #176

wants to merge 1 commit into from

Conversation

robbiespeed
Copy link

@robbiespeed robbiespeed commented Apr 13, 2024

This adds 3 new tests, 2 of which currently fail:

  • collects out of scope consumers (✅): Ensures abandoned Computed's (not consumed by anything, and no references) won't cause a memory leak.
  • collects inactive dynamic sources (❌): Ensures old dynamic sources that get replaced can be collected. Since the consumer isn't dirty some might believe this to be expected.
  • collects inactive dynamic sources after update (❌): Ensures old dynamic sources that get replaced can be collected after a update that could have marked the consumer dirty.

The impacts of the two failing tests are bounded, so it's more of a memory deficiency than a full on leak.

Required updating viteconfig to expose gc, doesn't appear to work inside the default pool type. Also WeakRef wasn't usable in spec files, hence adding the nested tsconfig. Doing so enforced strict mode on the spec file so a few further updates to the types were made to appease the type checker.

@robbiespeed
Copy link
Author

Something to consider is how strict these tests should be. There's optimisations that can be made if for example things are more lazily opened up for collection by GC. For example switching a complex branch of the graph from "live" to "not-live" can be expensive, so it might make sense to delay such an action, but doing so would keep some things around longer than necessary.

@@ -978,6 +978,7 @@ describe("type checks", () => {
TypeError,
);
expect(Signal.subtle.Watcher.prototype.watch.call(w, s)).toBe(undefined);
// @ts-expect-error
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli Apr 21, 2024

Choose a reason for hiding this comment

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

why expect error?

whenever I use ts-expect-error, I like to add a comment explaining why (not what (or the specific error -- the specific error is not relevant, but the why it exists is)!)

Copy link
Author

Choose a reason for hiding this comment

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

If you look below there's a bunch more pre-existing ts-expect-errors. Asides from that I think it's self explanatory in these cases why all of these are present. It's specifically testing areas with incorrect types to produce TypeErrors.

@@ -484,11 +484,11 @@ describe("Errors", () => {

describe("Cycles", () => {
it("detects trivial cycles", () => {
const c = new Signal.Computed(() => c.get());
const c: Signal.Computed<never> = new Signal.Computed(() => c.get());
Copy link
Collaborator

Choose a reason for hiding this comment

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

did something go wrong with inference here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes it's an infinite loop c is depending on itself. TS wasn't being ran in strict mode before this PR, as the tests are excluded from the main tsconfig.

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.

2 participants