robatwilliams / es-compat

*** DEPRECATED*** Check JavaScript code compatibility with target runtime environments
MIT License
66 stars 13 forks source link

Console logs mess up structured output from ESLint #76

Closed benasher44 closed 11 months ago

benasher44 commented 1 year ago

Hi there! Really enjoying this library so far, but unfortunately the console log that says which targets are being checked makes it difficult for CI tools to parse the output from eslint. Would it be possible to put these logs behind a DEBUG= env var or something? Thanks!

robatwilliams commented 1 year ago

What tool are you using that's parsing the text log from eslint? I wouldn't expect anything to be parsing this human readable output log; rather that tools would parse JSON output from ESLint.

It could be done neatly and conventionally using https://www.npmjs.com/package/debug

There is already a warn log for es-compat: No compatibility data for target family '${name}', so I'm not sure really if the one you're talking about (es-compat: checking compatibility for targets) is needed at all. The information on what targets is checked could be deduced using https://browserslist.dev/ and the warn log. However, the warn log is also a potential to confuse anything that's parsing the log, and I'm not sure how I would surface this warning to the user otherwise.

benasher44 commented 1 year ago

Oh yeah sorry it's these two:

es-compat: No compatibility data for target family 'and_uc'
es-compat: checking compatibility for targets {
  chrome_android: '113',
  firefox_android: '113',
  webview_android: '4.4.3',
  chrome: '79',
  edge: '111',
  firefox: '111',
  safari_ios: '12.2',
  opera: '96',
  safari: '14.1',
  samsunginternet_android: '20'
}

We're using reviewdog, which is parsing eslint output (with --format rdjson set for eslint)

benasher44 commented 1 year ago

Is it possible to detect the format flag? If so, that could be a good hint to skip this output. I find the output kind of nice — just an issue for our CI

robatwilliams commented 1 year ago

Does it use the -o / --output-file option too, or read rdjson from stdout?

It would seem fragile to assume that nothing except the machine readable format would appear on stdout. If this is the case, then any other lint plugin that uses console log/warn will need this log silencing fix.

I think the flag could be detected yes, via context parameter in the rule.

benasher44 commented 1 year ago

It doesn't use output-file. It reads it from stdout. It would seem fragile, but at least based on the plugins we have installed so far, it seems uncommon for eslint plugins to emit informational output.

robatwilliams commented 1 year ago

Agreed. Given ESLint supports JSON to stdout, it appears to encourage this use case of parsing structured output from stdout. So we shouldn't get in the way of that.

I think it should only log if using one of the formatters listed as human-readable from here https://eslint.org/docs/latest/use/formatters - so compact and stylish.

According to the page, the only way of specifying formatters is via CLI option, and there can only be one formatter. I can't immediately see how we can access that from the rule - https://eslint.org/docs/latest/extend/custom-rules#the-context-object . It feels like something rules shouldn't know about, so this isn't surprising.

I'm quite busy for the short term but if you want to start a conversation over on ESLint's repo on how we should log / how we can get what the current formatter is, then we might be able to agree a draft solution sooner for either one of us to implement.

benasher44 commented 1 year ago

What if we hide these messaged behind process.env.DEBUG or some other env var? It could be well-documented in the readme. One could argue that these messages fall in the category of "messages helpful when debugging failures but don't need to be seen all the time" and therefore would be a good fit for process.env

robatwilliams commented 1 year ago

I think "checking compatibility for targets" can go behind a debug flag, using https://www.npmjs.com/package/debug

The warning "No compatibility data for target family" is something the user should be aware of, otherwise they probably have false confidence that es-compat is checking things that it isn't. The simple solution for this would be to refer to https://github.com/mdn/browser-compat-data/tree/main/browsers from documentation.

benasher44 commented 1 year ago

How do you make the "No compatibility data…" warning go away? If that could be included in the message, that would be helpful

robatwilliams commented 1 year ago

It's because there are browsers on your browserslist for which MDN compatibility database doesn't have data. There's no current way other than removing from your browserslist.

benasher44 commented 1 year ago

Here's what the config looked like:

>0.2%, not dead, not ie <= 11, not op_mini all

So it seems like and_uc is in browserslist's db, but MDN doesn't have info on it. That's fine, but it's still difficult as a user to know what to do here, since technically it's not explicitly in my config, it's just in the browserslist db (but not in MDN).

benasher44 commented 1 year ago

The only other thing I can think of would be to use https://github.com/fyrd/caniuse data instead, though that is a much bigger ask.

robatwilliams commented 11 months ago

Resolved by #83

robatwilliams commented 11 months ago

Released in https://github.com/robatwilliams/es-compat/releases/tag/v3.1.0

@benasher44