Proposal: Remove logging from Solid UI and add it as a module that is exposed through the data browser

[megoth]

I'm currently working on the log module, which exposes a set of methods that mostly do one of three things, depending on the environment:

1. if the environment has the document object (e.g. is a browser), it will look for an element with id "status" and append lines to it with time, type of log message and the message whenever the methods are called with a message, or
2. if the environment supports console (e.g. Node), it will use console.log to output the messages, or
3. do nothing

(The other methods that do not log messages are various configuration methods.)

I think it's weird that the module is dependent on a global element somehow being available, and think it would be better to expose the API through a module that is exposed through the data browser, probably through the DataBrowserContext interface that is made available to views through methods such as render. (It is currently defined in pane-registry.)

Data browser implementations would handle logging differently, but expose the same API. This module could then be passed to methods in Solid UI that requires logging, making the need for log module in Solid UI obsolete.

[michielbdejong]

👍 agree that if you use the UI global which this module exports, it would make sense if you can set UI.logging = ..., or even better new UI({ logging: ... }). However, I think we also want to allow depending software projects to only for instance:

import { askName } from 'solid-ui'

In that case, how would you configure the logging?

I think simple but big step forward would be:

• search/replace console.log to debug.log
• debug.log function checks if process.env.DEBUG or window.DEBUG is truthy, and if so, calls console.log.

Nits:

that is exposed through the data browser

Or through whichever other project (other than the data browser) happens to use Solid-UI as a components library, right? I think we want to remove any places where Solid-UI depends on the presence of the outlineManager, paneRegistry, etc.

Data browser implementations

We can even generalise this to any software projects that depend on Solid UI, without assuming any knowledge about what those projects are.

[megoth]

However, I think we also want to allow depending software projects to only for instance:

import { askName } from 'solid-ui'

In that case, how would you configure the logging?

This proposal would require a complex setup of those functions, correct, so maybe the cost is to high. Another course would be to allow overriding the default behavior of the log module by replacing it with your own logging module, which I guess is what you meant with the following?

👍 agree that if you use the UI global which this module exports, it would make sense if you can set UI.logging = ..., or even better new UI({ logging: ... }).

Some functionality that allows data browser implementations to override the logging module so that they decide how it's handled is certainly a possibility (e.g. dependency injection), and I agree that it's a better approach to keep the complexity of signatures such as askName low.

I think simple but big step forward would be:

* search/replace console.log to debug.log

* debug.log function checks if `process.env.DEBUG` or `window.DEBUG` is truthy, and if so, calls console.log.

Is debug a new module? In general I prefer to avoid the use of global variables such as window.DEBUG, so let's find another approach.

Nits:

that is exposed through the data browser

Or through whichever other project (other than the data browser) happens to use Solid-UI as a components library, right? I think we want to remove any places where Solid-UI depends on the presence of the outlineManager, paneRegistry, etc.

I agree that Solid UI should not depend on concepts such as panes/views and OutlineManager (which is a implementation detail of the data browser).

Data browser implementations

We can even generalise this to any software projects that depend on Solid UI, without assuming any knowledge about what those projects are.

Yup.

[michielbdejong]

Some functionality that allows data browser implementations to override ...

Same nit as before ;)

Is debug a new module? In general I prefer to avoid the use of global variables such as window.DEBUG, so let's find another approach.

Well, what if we actually just use https://www.npmjs.com/package/debug#web-browser

[megoth]

Some functionality that allows data browser implementations to override ...

Same nit as before ;)

My point is that it's weird for Solid UI to rely on a global element with id "status". This behavior is something data browsers should be able to override. One approach for that is to allow some sort of dependency injection where data browser (or any type or project) can set their log module so that all calls to methods in the Solid UI log module are routed to the injected log module.

Is debug a new module? In general I prefer to avoid the use of global variables such as window.DEBUG, so let's find another approach.

Well, what if we actually just use https://www.npmjs.com/package/debug#web-browser

Sure, if we deem it valuable enough given its cost in bytesize and security.

My main point was that we should avoid global variables.

[michielbdejong]

I would consider the debug yes/no global more of an environment setting. I don't see any other way to make

import { askName } from 'solid-ui'

work.

So let's start by centralising all console.log into a single place where we can switch it on and off, and then for starters we just use an environment setting to do so, and if we find a cleaner way to do it in the future, then that will be a change in a single place (since the 166 places in the code where console.log is called will then already have disappeared).

[megoth]

I would consider the debug yes/no global more of an environment setting. I don't see any other way to make

import { askName } from 'solid-ui'

work.

Environment setting, yes, global setting via the global scope, no. There are ways to work around these limitations, and good reasons for having them.

So let's start by centralising all console.log into a single place where we can switch it on and off, and then for starters we just use an environment setting to do so, and if we find a cleaner way to do it in the future, then that will be a change in a single place (since the 166 places in the code where console.log is called will then already have disappeared).

I would agree with this if it where not for the current weird behavior of log module doing DOM manipulation (hence this issue). Centralizing all console.log would probably be overkilling it, since I suspect a lot of them are not intended from the developers side to manipulate a global DOM element.

[michielbdejong]

Centralizing all console.log would probably be overkilling it, since I suspect a lot of them are not intended from the developers side to manipulate a global DOM element.

I'm not implying that they should manipulate a global DOM element. I'm just saying they should not be exist in the way they exist now. In other words, I think we should activate https://eslint.org/docs/rules/no-console - I think we agree on that goal, right?

So then a simple solution is to add a function that we can call instead of it.

As a straw-man, I quickly did that here: https://github.com/solid/solid-ui/pull/267

But let's discuss it at Monday's standup, sounds like you're blocked on it but I don't think I really understand what your question is.