-
Notifications
You must be signed in to change notification settings - Fork 8
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
Clean up Unused Observable Stores (v12.0.0) #187
Conversation
beforeEach(() => { | ||
// clean up any tram-one properties between tests | ||
window['tram-space'] = undefined; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLDR, I didn't notice this existed, ended up wasting a lot of time trying to reason around the global 'tram-space' in the other test files, and ended up making this clean up part of the startApp
method.
// start the app | ||
await startAppAndWait(); | ||
|
||
// previously the working branch indices would have long recursive chains of branches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in reality, this was a side-effect of not cleaning up the tram-space
in the unit tests (done now in startApp
, but I was concerned so I ended up writing this test.
I believe this is also happening when hot-reloading occurs - I haven't noticed any significant side-effects from that, but we should look into it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue Created: #188
const { default: userEvent } = require('@testing-library/user-event'); | ||
const { startApp } = require('./test-app'); | ||
const { startAppAndWait } = require('./test-helpers'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using startAppAndWait
here instead of startApp
- while not all tests need this, I'd like to slowly start making this the standard. See integration-tests/test-helpers.ts to understand what this does.
@@ -89,7 +94,7 @@ describe('Tram-One', () => { | |||
}); | |||
|
|||
// clear the input | |||
userEvent.type(getByLabelText(container, 'New Task Label'), '{selectall}{backspace}'); | |||
userEvent.clear(getByLabelText(container, 'New Task Label')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't realize that this clear
method existed, but yeah, it very specifically says it selects the text and then deletes it
*/ | ||
|
||
describe('Tram-One', () => { | ||
it('should clean up stores for elements that are no longer rendered', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the more nitty-gritty, internals-aware version a test that we have in the regressions suite. This was more of a stepping stone for those other tests, and while slightly redundant, it acts as a good sanity check if those other tests end up failing.
In reality, if the internals of the app change, this could end up needing to be scrapped, but we'll cross that bridge when we get there.
// change the value of the input | ||
fireEvent.change(getByLabelText(container, 'Store Generator'), { target: { value: 1 } }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually wish we could do a userEvent
here, but sadly the arrow interactions don't update the input value. If we wanted to make this work we'd have to make a change to the user-events repo - something similar to this: https://github.com/testing-library/user-event/blob/ee062e762f9ac185d982dbf990387e97e05b3c9d/src/pointer/pointerPress.ts#L254-L327
/** | ||
* Element to verify non-standard input controls, and also verify memory leak type issues | ||
*/ | ||
const elementstoregenerator: TramOneComponent = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See PR summary for the screenshots of this component.
:root { | ||
color-scheme: dark light; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the test-app to use browser native dark / light theme. This should respect the system preferences now!
This is also a change we'd like to make to the generated tram-one apps: Tram-One/tram-one-express#110
/** | ||
* decorated startApp function that ensures that the app's mutation observers | ||
* have kicked in before starting to interact with the app | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
often times in the unit tests the mutation observers would try to handle multiple interactions at the same time. This did not occur when using the test-app locally, but was a side-effect of jest/testing-library not waiting for the mutation observer to catch up as it was interacting with the app (and, to be fair, rightfully so).
This decorated startApp
function starts the app, and then waits for one of our stores (which is updated by the mutation observer) to be populated. This allowed the tests to pass as expected.
@@ -11,7 +11,7 @@ import { | |||
restoreWorkingKey, | |||
} from './working-key'; | |||
import observeTag from './observe-tag'; | |||
import processEffects from './process-effects'; | |||
import processHooks from './process-hooks'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed this file / function, which better reflects what it does now (since it processes both effects and state!)
export const clearEffectStore = (effectStoreName: string) => { | ||
const effectStore = getEffectStore(effectStoreName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was unclear, and threw me off, so renamed this to be explicitly the EffectStore's Name.
/* | ||
* The KeyStore in Tram-One is a basic key-value object | ||
* that needs to be persisted in the globalSpace. | ||
* | ||
* Currently this is used with useStore and useGlobalStore to keep | ||
* track of what stores need to be cleaned up when removing elements | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is heavily inspired by the EffectStore.
This store keeps track of store keys (e.g. "app[{}]/test[{}]"
, and how many observers they have (just a number).
If the number of observers goes down to zero after removing an element from the DOM, then we remove the store.
/* | ||
* The KeyQueue in Tram-One is a basic list of keys | ||
* that needs to be persisted in the globalSpace. | ||
* | ||
* Currently this is used with useStore to keep track of what | ||
* stores need to be associated with generated elements | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This KeyQueue (while different functionally) is similar to the EffectQueue. It is used to determine what store keys are generated during element generation, and tie it back to the element.
The name KeyQueue
is intentionally generic, as (hopefully in the near future) we may consolidate the EffectQueue into this, and the EffectStore into the KeyStore (see #189)
const defaultWorkingKey = () => | ||
({ | ||
// list of custom tags that we've stepped into | ||
branch: [], | ||
// map of branches to index value (used as a cursor for hooks) | ||
branchIndices: { | ||
'': 0, | ||
}, | ||
} as WorkingkeyObject); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build a new object (previously we would always point to the same object, which created stale data between tests).
// store keys in the node we just built | ||
tagResult[TRAM_TAG_STORE_KEYS] = newKeys; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what associates the observable keys with the element. Basically, after we've generated all the elements (and called any store hooks), we put those keys on the node, which can then be picked up by the mutation observer.
try { | ||
if (elementToGiveFocus.setSelectionRange !== undefined) { | ||
elementToGiveFocus.setSelectionRange( | ||
removedElementWithFocusData.selectionStart, | ||
removedElementWithFocusData.selectionEnd, | ||
removedElementWithFocusData.selectionDirection | ||
); | ||
} | ||
} catch (exception) { | ||
// don't worry if we fail | ||
// this can happen if the element has a `setSelectionRange` but it isn't supported | ||
// e.g. input with type="range" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's slightly baffling that range inputs have a setSelectionRange
method, but don't actually support it, it's kinda strange, but there's no real way to avoid this other than put a try-catch around it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could potentially catch the specific exception, and throw other exceptions we don't expect (although honestly I don't think we want to throw any exception, this is a very non-critical path for the app).
// if we weren't passed in a key, this is a local obserable (not global), | ||
const isLocalStore = !key; | ||
if (isLocalStore) { | ||
// if this is local, we should associate it with the element by putting it in the keyQueue | ||
getKeyQueue(TRAM_KEY_QUEUE).push(resolvedKey); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what adds this observable's key to the queue, which is later picked up and added to the element attributes.
src/mutation-observer.ts
Outdated
const cleanUpObservableStores = () => { | ||
const observableStore = getObservableStore(TRAM_OBSERVABLE_STORE); | ||
const keyStore = getKeyStore(TRAM_KEY_STORE); | ||
Object.entries(keyStore).forEach(([key, observers]) => { | ||
if (observers === 0) { | ||
delete observableStore[key]; | ||
} | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the above functions we increment or decrement the number of observers associated with a key. If at the end of the day there are zero observers, this is where we delete the store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we now also delete the key from the keyStore
// don't lose track that this is still a tram-one element | ||
tagResult[TRAM_TAG] = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we totally forgot to do this before 😅
const keyStore = getKeyStore(TRAM_KEY_STORE); | ||
Object.entries(keyStore).forEach(([key, observers]) => { | ||
if (observers === 0) { | ||
delete observableStore[key]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should also delete the key from the keystore (otherwise we'll keep trying to delete these, even when they are already gone).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one question but otherwise looks good!
/** | ||
* clear the key queue | ||
* usually called when we want to empty the key queue | ||
*/ | ||
export const clearKeyQueue = (keyQueueName: string) => { | ||
const keyQueue = getKeyQueue(keyQueueName); | ||
|
||
keyQueue.splice(0, keyQueue.length); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is slice used here to provide a mutation on a reference or could we not do some assignment? Or maybe I'm getting caught up in the naming of this function (this doesn't look feel a true "clear" operation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe splice will allow us to empty the array. To be honest I did look this up, and there was also an option to just reassign it to an empty array, but we need to make sure that other references to this variable are cleared out, hence splice. Performance wise it was documented as the same as reassignment
Summary
As a side-effect of working on RedditComber a considerable performance slow down was found when interacting with the app for a period of time. This was determined to be a memory leak, caused by local stores remaining in the app even when those elements were removed from the page.
This PR fixes #185 (and additionally fixes #184)
For anyone that wants a demonstration / detailed walk-through of this PR, feel free to reach out to me on the Tram-One Discord
Changes
All the changes listed below are detailed in the comments, treat these as a high level breakdown.
Bump Major - v12.0.0
Because we now blow away state when an element is removed, this could be considered as a breaking change. While it was never intended that this state would persist in this way, it could certainly be seen as a feature. In reality, it makes more sense for people to use
useGlobalStore
if they want this persistent state.If it turns out that this feature was super useful, we could consider making a new state hook, that was local to the component, but never blown away.
Main Changes
KeyStore
andKeyQueue
, which keeps track of store keys associated with elementsuseStore
when an element is removedAuxiliary Changes
input
types (e.g. range) would break when trying to reattach focus.Testing Changes
and then can be changed to create or remove stores (each button is an element with a local count):
Performance / Size Changes
See range-and-leaks...range-and-leaks-artifacts
The TLDR is that the size increases by a small amount, and the performance test is just ever so slightly slower, but not significantly.
This may seem counter-intuitive given that this PR was intended to remove performance issues, but the nature of the existing performance tests don't stress this kind of behavior.
Todo
Checklist