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

Ambiguous Usage of Unhook method #88

Open
Coteh opened this issue May 31, 2021 · 2 comments
Open

Ambiguous Usage of Unhook method #88

Coteh opened this issue May 31, 2021 · 2 comments

Comments

@Coteh
Copy link

Coteh commented May 31, 2021

Hi,

I am just curious about the intended usage of Unhook method.

Currently, I have the following inside my component:

const [logs, setLogs] = useState<Message[]>([]);

useEffect(() => {
        Hook(
            window.console,
            (log) => setLogs(currLogs => [...currLogs, log]),
            false,
        );

        return () => {
            Unhook(window.console as any);
        }
}, []);

This matches the usage example in the README and I think this would pass in a JavaScript project. However, in TypeScript, I need to cast window.console to any type when passing it to Unhook otherwise the following error occurs:

Argument of type 'Console' is not assignable to parameter of type 'HookedConsole'.
  Property 'feed' is missing in type 'Console' but required in type 'HookedConsole'.  TS2345

    21 | 
    22 |         return () => {
  > 23 |             Unhook(window.console);
       |                    ^
    24 |         }
    25 |     }, []);
    26 | 

Casting as any would be an acceptable solution (but not a desirable one) to me, but I also noticed that Hook method returns the console back as a HookedConsole object (whose type is basically the same as Console but with feed property).

Is there any difference in using the return value of Hook and passing that into Unhook instead? Like this:

const [logs, setLogs] = useState<Message[]>([]);

useEffect(() => {
        const hookedConsole = Hook(
            window.console,
            (log) => setLogs(currLogs => [...currLogs, log]),
            false,
        );

        return () => {
            Unhook(hookedConsole);
        }
}, []);

After viewing the implementation, this should work the same as the previous, since window.console has the feed property injected into it and a reference to the same object is returned from the Hook method. But this doesn't seem to be documented. Is there a caveat to this approach? Or could the documentation be updated to reflect this approach instead? This is more desirable for a TypeScript project IMO since it avoids a cast to any.

edit: User could also cast window.console to HookedConsole directly, but this requires the user to have read the code and understood what it does to the window.console object to verify that this type conversion is valid. Not sure what the best approach is here from documentation standpoint.

@Coteh
Copy link
Author

Coteh commented May 31, 2021

I also realize in #57 it was mentioned that the project did not originally ship with type definitions. Still, I thought I'd mention this as something to consider.

@pixelass
Copy link
Contributor

The docs are misleading. I prepared a PR for this #113 .

You should assign the return value of Hook and then unhook that assignment.
This is similar to setTimeout or setInterval.

useEffect(() => {
-    Hook(
+    const hookedConsole = Hook(
      window.console,
      (log) => setLogs((currLogs) => [...currLogs, log]),
      false
    )
-    return () => Unhook(window.console)
+    return () => Unhook(hookedConsole)
  }, [])

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

No branches or pull requests

2 participants