Refactor to TypeScript and ship types #71
Conversation
There was a problem hiding this comment.
Thanks a lot for picking this up!
I haven't had the time doing a full review. But as I have some draft comments pending since a few days I felt sending them as-is is better than letting you wait longer.
Additionally to the inline comments, it would be great if you could convert the tests to TypeScript as well. That gives us test coverage for the types.
| @@ -22,19 +22,23 @@ window.addEventListener('storage', function ({ key, newValue }) { | |||
| return; | |||
| } | |||
|
|
|||
| localStorageCache.set(key, jsonParseAndFreeze(newValue)); | |||
| localStorageCache.set(key, jsonParseAndFreeze(newValue as string)); | |||
There was a problem hiding this comment.
Can we guard against newValue being null instead of doing a type assertion?
addon/index.ts
Outdated
| @@ -79,7 +84,7 @@ export function initalizeLocalStorageKey(key) { | |||
| managedKeys.add(key); | |||
| localStorageCache.set( | |||
| key, | |||
| jsonParseAndFreeze(window.localStorage.getItem(key)) | |||
| jsonParseAndFreeze(window.localStorage.getItem(key) as string) | |||
There was a problem hiding this comment.
I think we should handle the null case explicitly instead of doing a type assertion.
|
Still working on these fixes. One question--when the decorator is used without arguments ( Are you able to shed any light on that logic? |
|
Also, where does that |
The issue might be caused by wrong return type declaration for I expect that the current function signature also causes a TypeScript error when the decorator is used. |
Fixes #67