Style error information

https://eslint.org/docs/rules/no-throw-literal

Understanding:
`throw`:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statem...

`new Error`:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global...

Remark

Code Improve. Less hamful

How to Review

## 1. Apply Patch
## 2. Review Code Changes
## 3. Confirm no Code Standard Errors
yarn & yarn lint:core-js-passing
## 4.1 If `NO` errors, mark the issue as `Reviewed & tested by the community` (Don't be shy, we're all friendly)
## 4.2 If `HAS` errors, fix it and upload a new patch (Just do it and you can!!!)

Background

- #2912962: Step 1 JS codestyle: [meta] Fix JS coding standards in core

- We adapted the airbnb coding standard (#2815077: Adopt airbnb javascript style guide v14.1 as new baseline javascript coding standards for Drupal 8 core and contrib), but we are not fully compliant to it yet.

More Information

- Using ES6 in Core
https://www.drupal.org/node/2815083

- To find JS code standard errors stats
yarn & yarn lint:core-js-stats

Valuable Follow-up

- core/modules/quickedit/js/quickedit.js throws JS error. It's not in testcases. It might requred `t` function to translate the string.

CommentFileSizeAuthor
#9 2914713-9.patch2.68 KBGrandmaGlassesRopeMan
eslint-no-throw-literal.patch2.7 KBdroplet

Comments

droplet created an issue. See original summary.

droplet’s picture

droplet commented
Issue summary: View changes
nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue tags: +JavaScript clean-up

The string was thrown because there is no need for a stack trace about this message. It might confuse the user that this is a js error when it's really a HTML issue. It's only about warning the user that an attribute is missing.

I'm not sure if we should change it. Might be enough to ignore this one specifically. In any case I don't really care either way.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
commented

Could we use console.error() instead?

droplet’s picture

droplet commented

Hmm, we might throw a `ReferenceError` instead
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global...

It was introduced by me intentionally (https://www.drupal.org/node/2551373#comment-11927326). But removing by @drpal ( https://www.drupal.org/node/2551373#comment-12180188 ). He didn't explain why but I guess the same stack issue?

@drpal removed the `t` also but I think we should add it back. The CORE tests that faced to CORE developers. It seems okay to drop translations. But for other parties, developers cannot speak English is acceptable. (follow-ups)

`Throw` stopped the script, that's diff to `console`. And I don't quite like the non-ES standard `console` in production env. Developers often stripe it with https://www.npmjs.com/package/gulp-strip-debug or UglifyJS2 (drop_console=true)

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented

@droplet

Yeah. I remember this. I don't actually remember why I dropped the throw new Error(), maybe I intended to use console.error()?

dawehner’s picture

dawehner
Primary language German
commented

So what should do do? The documentation already talks about errors. For me at least there is something clearly wrong in your development when this happens, so you should see it as visible as possible.
In other words +1 for the approach.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
StatusFileSize
new 2914713-9.patch2.68 KB

- reroll.

I'm good with this approach.

dawehner’s picture

dawehner
Primary language German
commented

@drpal
One question, what happens to other JS on the page if this error is thrown? Is just quickedit broken or is there potential other behaviour not longer applied?

nod_’s picture

nod_
Primary language French
Location Lille
commented

the quickedit behavior crashes and the other behaviors are executed. We have a guard in attachBehaviors to prevent one behavior from stopping everything.

It's only quickedit that might be in a broken state.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Thank you nod_! I think this is a totally acceptable behaviour then.

xjm’s picture

xjm
she/her
Primary language English
commented

So in terms of t() and exception messages, for PHP, we have a policy of not translating exception messages for cleaner debugging. There's no t() in HEAD so I think that's out of scope here -- is there a followup issue I should comment on?

  • xjm committed 5951ea0 on 8.5.x 
    Issue #2914713 by droplet, drpal, dawehner, nod_: JS codestyle: no-throw...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

On the point of what error objects to use (console.error(), ReferenceError, etc.) adopting a best practice would change more than just this one line. The current patch is the smallest possible change to make core comply with this rule, and given @nod_'s clarification in #11, I think it's safe to go ahead and commit this small cleanup as-is and then explore other changes in followup issues that would apply to all of core.

Before:

no-use-before-define: 75
max-len: 70
camelcase: 65
no-restricted-syntax: 54
no-shadow: 42
new-cap: 18
default-case: 16
no-continue: 9
no-new: 9
no-throw-literal: 1

After:

no-use-before-define: 75
max-len: 70
camelcase: 65
no-restricted-syntax: 54
no-shadow: 42
new-cap: 18
default-case: 16
no-continue: 9
no-new: 9

Committed and pushed to 8.5.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.