Added MemoryStore shutdown method

[codyebberson]

Related Issues

Fixes #320

What Does This PR Do?

Added

Added MemoryStore.shutdown which clears the timer and allows applications to completely stop a running instance of MemoryStore.

Checklist

• The issues that this PR fixes/closes have been mentioned above.
• What this PR adds/changes/removes has been explained.
• All tests (npm test) pass.
• All added/modified code has been commented, and
  methods/classes/constants/types have been annotated with TSDoc comments.
• If a new feature has been added or a bug has been fixed, tests have been
  added for the same.

[gamemaker1]

Thank you for taking the time to make this PR, this looks great! Could you please also add the shutdown method to the Store interface in source/types.ts too? That way other stores can add it too.

[nfriedly]

Could you please also add the shutdown method to the Store interface in source/types.ts too?

That's a good idea! We should make it optional, though I don't want other stores to be required to implement it. Also, it should probably be async so that stores where the shut down is not instantaneous can indicate when it's complete.

[codyebberson]

Can do. Something like this?

shutdown?: () => Promise<void> | void

[gamemaker1]

This is great!

Sorry I didn't mention this earlier, but I think we should add an external test for this too - adding a shutdown method that prints 'Shutdown successful' (to the store here) and call the function when a request is received (here); and then make the same changes to this file too.

We should also add documentation about the shutdown method to the wiki.

After that, this PR should be good to go!

[NoahAndrews]

Do we need the | void? I'd have thought the question mark was enough.

[codyebberson]

@gamemaker1 Of course.

call the function when a request is received

I'm not sure I understand. Call the shutdown method from inside the request?

@NoahAndrews

Do we need the | void? I'd have thought the question mark was enough.

The | void applies to the return type of the optional method, meaning that shutdown can return a Promise<void> or void. I'm happy to change it to simply always return a Promise<void>.

[gamemaker1]

I'm not sure I understand. Call the shutdown method from inside the request?

Oof I realise that's really silly to do, sorry about that. I think a better way would be:

• first add a shutdown method to the TestStore here that prints test-store: shutdown successful.
• then export a store variable (with this value) from source/app.ts.
• then call the shutdown function on the store once all the test requests are made (here), and check that the message test-store: shutdown successful has been printed if the store was an instance of TestStore. If not, we don't need to check as the memory store's shutdown method is already checked by the library tests. See this test for an example of checking if console.log has been called.

You would also need to do the same for the named-import external test for ts-esm here.

The | void applies to the return type of the optional method, meaning that shutdown can return a Promise or void. I'm happy to change it to simply always return a Promise.

Please keep the | void, it allows the function to be synchronous. If the function's allowed return type is only Promise<void>, then it forces the function to be asynchronous.

[gamemaker1]

This looks good to go!

@nfriedly what do you think?

[nfriedly]

Thanks, this is great work! Sorry I've been out of the loop for a couple of days.

I think we should make the test call expect(setTimeout).toHaveBeenCalledWith(...); but the other things I mentioned are minor points that don't necessarily need to be changed.

[nfriedly]

All of these console.log tests don't really seem necessary to me, but I know it was @gamemaker1's request and I guess it's not really hurting anything either. 🤷

[gamemaker1]

LGTM!

[nfriedly]

Thank you, this is a good addition! I'll get a release out shortly.