NPM Resource Changed PR-4.3-dev Pending

User tests: Successful: 0 Unsuccessful: 0

avatar laoneo
laoneo
13 Mar 2023

Summary of Changes

We should not log into the console in Javascript.

Testing Instructions

Open the drone JS code style check.

Actual result BEFORE applying this Pull Request

A warning is logged.
image

Expected result AFTER applying this Pull Request

No warning.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

avatar joomla-cms-bot joomla-cms-bot - change - 13 Mar 2023
Category JavaScript Repository NPM Change
avatar laoneo laoneo - open - 13 Mar 2023
avatar laoneo laoneo - change - 13 Mar 2023
Status New Pending
avatar wilsonge
wilsonge - comment - 13 Mar 2023

What's the point of catching the promise error if you are doing this? Personally I think in the exception catching part we probably do want a console.log so we have some insight but we don't necessarily need to stop the script in it's entirety? (i.e. an eslint ignore is probably all we want here?).

avatar laoneo
laoneo - comment - 14 Mar 2023

Console log is a developer thing and should not be used in production code. Either throw an exception or silently continue. I let @obuisard decide, but console.log should be removed.

avatar obuisard
obuisard - comment - 14 Mar 2023

Thanks, Allon.
The purpose here was to leave information about what is going on because the Shepherd library throws an error when a step target is not found on a page. This could happen, for instance, when running the article tour and workflows are enabled. The status field id, in this case, is no longer present on the page. Shepherd throws an error because the target is not found. However, the tour is not cancelled, the user is able to resume and finish the tour (when Shepherd throws a 'not found target' error, the step still shows, without arrows and the step is centered).

avatar laoneo
laoneo - comment - 17 Mar 2023

So then we can remove the console log?

avatar wilsonge
wilsonge - comment - 17 Mar 2023

Why? What benefit is there to silently swallowing errors? Ideally we’d have a mechanism to send them server side, but given we have nothing console is better than nothing

avatar laoneo
laoneo - comment - 17 Mar 2023

What do you gain? We don't log to console in core so why should we do now an exception?

avatar obuisard
obuisard - comment - 17 Mar 2023

My idea was to give a more 'readable' way to understand why a step would not reach its target during a tour.
Useful for people who create tours and useful for users who want to understand why a step is acting differently than it should (if they are curious and know about the browser consoles).

However, the Shepherd library does throw an exception nonetheless, so, this added information could be removed.
Will remain the Shepherd error in any case - just raw error.

Note that when a CSS selector is invalid, we do use the messaging mechanism to alert the user in the admin console and stop the tour.

avatar laoneo
laoneo - comment - 17 Mar 2023

Really I do not care how you will remove it, but at the end, console.log must go away.

avatar wilsonge
wilsonge - comment - 17 Mar 2023

But we do use console.log regularly around core. Go search for it…

avatar laoneo
laoneo - comment - 17 Mar 2023

I did a search and only the build scripts are logging on node. Or where did you find an occurrence?

avatar Fedik
Fedik - comment - 17 Mar 2023

There should be either an exception or a console log, the error should not be hidden.
I am fine with anything.

However with current changes we have to be sure the script working as intended, an Exception may break some routinee.

avatar laoneo laoneo - change - 18 Mar 2023
Labels Added: NPM Resource Changed PR-4.3-dev
avatar laoneo
laoneo - comment - 18 Mar 2023

The error is now ignored. Should the error be printed to Joomla.renderMessages?

avatar laoneo
laoneo - comment - 22 Mar 2023

@obuisard can you look for an alternative if you want to show the error to the end user? console.log must be removed, that's why we have this linting rule in place and there is no other occurrence in the whole CMS which prints something to the browser console.

avatar obuisard
obuisard - comment - 29 Mar 2023

I agree with doing nothing here. The library Shepherd will generate an error anyway.

avatar obuisard obuisard - close - 29 Mar 2023
avatar obuisard obuisard - merge - 29 Mar 2023
avatar obuisard obuisard - change - 29 Mar 2023
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2023-03-29 17:09:10
Closed_By obuisard
avatar obuisard
obuisard - comment - 29 Mar 2023

Thanks Allon @laoneo

Add a Comment

Login with GitHub to post a comment