samdenty / console-feed

Captures console.log's into a React Component 🔥
https://stackblitz.com/~/github.com/samdenty/console-feed
MIT License
670 stars 109 forks source link

Ambiguous Usage of Unhook method #88

Open Coteh opened 3 years ago

Coteh commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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)
  }, [])