Style error information

https://eslint.org/docs/rules/no-restricted-globals

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
cd core/ && yarn & yarn lint:core-js-stats

Valuable Follow-up

- N/A

CommentFileSizeAuthor
#10 2983373-10.patch9.44 KBGrandmaGlassesRopeMan
#7 interdiff-2983373-2-7.txt882 bytesApacheEx
#7 2983373-7.patch9.43 KBApacheEx
#2 2983373-2.patch9.86 KBApacheEx

Comments

drpal created an issue. See original summary.

ApacheEx’s picture

ApacheEx commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2983373-2.patch9.86 KB

Here is a patch. I have also run yarn run build:js.

Status: Needs review » Needs work

The last submitted patch, 2: 2983373-2.patch, failed testing. View results

m1r1k’s picture

m1r1k commented 
+++ b/core/misc/displace.es6.js
@@ -122,7 +122,7 @@
+      if (Number.isNaN(displacement)) {

Seems like babel does not provide polyfill? Because Number.isNan is ES6 feature and in disaplace.js it is not changed

m1r1k’s picture

m1r1k commented

Styles were fixed, but what to do with Number.isNan polyfill?
How do you usually proceed guys? :)
1) manual polyfill
2) import 'core-js/es6/number';
3) etc

ApacheEx’s picture

ApacheEx commented

thanks for your investigation. IE doesn't support Number.isNaN, so yeah perhaps we need to polyfill it.
I will take a look today later.

ApacheEx’s picture

ApacheEx commented
Status: Needs work » Needs review
StatusFileSize
new 2983373-7.patch9.43 KB
new interdiff-2983373-2-7.txt882 bytes

I was trying to use https://babeljs.io/docs/en/babel-plugin-transform-runtime,
it will add polyfills automatically only when needed (based on browsers from babel-present-env).
But, that's something which depends on require or import - we don't support it ¯\_(ツ)_/¯

I see 2 ways:
1) ignore it by eslint-disable-next-line
2) add https://polyfill.io/ or properly configure https://babeljs.io/docs/en/babel-polyfill

currently, I'd prefer the first option. Any other thoughts?

m1r1k’s picture

m1r1k commented

as @drpal mentioned somewhere in other tasks, it is quite costy to add polyfills in such cases, because they will be added to all separate files and we can handle duplication while we transpile each file separately

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/misc/displace.es6.js
@@ -122,6 +122,7 @@
+      // eslint-disable-next-line no-restricted-globals

Nice. I think this fine for now.

@m1r1k & @ApacheEx - You are correct. Right now we don't have a solid plan for how we can handle polyfill support. If you want to open a followup where we can discuss an approach that would be wonderful.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
StatusFileSize
new 2983373-10.patch9.44 KB

- I forgot this minor reroll due to changes to core/.eslintrc.passing.json.

ApacheEx’s picture

ApacheEx commented

Thanks for reroll.
Here is a followup #2985495: JS: Polyfills support

  • lauriii committed 258a27b on 8.7.x 
    Issue #2983373 by ApacheEx, drpal, m1r1k: JS codestyle: no-restricted-...

  • lauriii committed d1b8295 on 8.6.x 
    Issue #2983373 by ApacheEx, drpal, m1r1k: JS codestyle: no-restricted-...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Committed 258a27b and pushed to 8.7.x. Cherry-picked d1b8295 and pushed to 8.6.x. Thanks! ✨

Status: Fixed » Closed (fixed)

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