tolitius / mount

managing Clojure and ClojureScript app state since (reset)
Eclipse Public License 1.0
1.22k stars 88 forks source link

Logger incompatible with latest goog.log #121

Open jwr opened 3 years ago

jwr commented 3 years ago

After switching to ClojureScript 1.10.844:

WARNING: Wrong number of args (1) passed to goog.log/getLogger at line 9 app/web/js/out/mount/tools/logger.cljc
WARNING: Wrong number of args (2) passed to goog.log/error at line 18 app/web/js/out/mount/tools/logger.cljc
WARNING: Wrong number of args (2) passed to goog.log/info at line 19 app/web/js/out/mount/tools/logger.cljc

But, actually, does mount really need a logger module at all? Seems to me like simpler would be better in this case: the simpler the library, the more resilient it is.

tolitius commented 3 years ago

the simpler the library, the more resilient it is

do agree with this. the role that logger plays is to notify when states go up and down as well as report errors that occur

the reason this namespace exists is to make sure we can consistently do that 👆 in any hosted environment using logging/ printing tools that apply in that environment

one thing that kind of leaks out is goog.log which is an implicit dependency since cljs is based on closure. it's like if java would move forward and broke compatibility on the language level in the JVM universe.

I think the right way to do here is to update mount/tools/logger.cljc to support a new version of closure's goog.log API

jwr commented 2 years ago

Since this issue blocks updates of ClojureScript for me, I tried to take a closer look and update mount. But I have no idea how and I do not understand the warnings.

goog.log.error seems to take two mandatory parameters and a third optional one:

/**
 * Logs a message at the goog.log.Level.SEVERE level.
 * If the logger is currently enabled for the given message level then the
 * given message is forwarded to all the registered output Handler objects.
 * @param {?goog.log.Logger} logger
 * @param {!goog.log.Loggable} msg The message to log.
 * @param {*=} exception An exception associated with the message.
 */

goog.log.error = function(logger, msg, exception = undefined) {
  if (goog.log.ENABLED && logger) {
    goog.log.log(logger, goog.log.Level.SEVERE, msg, exception);
  }
};

So, why the warnings? Are they problematic/harmful? Do I need to worry about them at all?

I would still respectfully suggest that removing the logging from mount would make for a better library, given that maintaining this part seems to be problematic. Also, is there a way to contribute to mount maintenance (github sponsors, for example)?

tolitius commented 2 years ago

PR is definitely welcome for this one! I have not looked at the new goog.log. still think this is valuable, especially for error reporting since there are plenty of silent problems in JS universe. hope you have time to look more into it, or/and maybe someone else can?