Allow specifying provided polyfills (#2)

[sobstel]

Allow to whitelist particular non-syntax features as polyfills, so they aren't reported as problems.

{
  "rules": {
    "ecmascript-compat/compat": [
      "error",
      {
        "polyfills": [
          "Array.prototype.flat"
        ]
      }
    ]
  }
}

[robatwilliams]

Hi, thanks for taking a look at this.

In principle I don't agree with doing string inclusion checks to achieve this.

How about something like this?

1. On every rule definition, add an optional property polyfill property with the text that must go in the rule options.
2. In rule.js:create(), filter out the rules.

Once past draft state, also:

1. Add unit tests for every polyfillable rule under valid: in the es2xxx.spec.js files (see here for how to specify options).
2. Schema should validate that only known polyfills are specified. The valid strings could be pulled off the rule definitions programmatically, but it's perhaps better just to hard code all the strings here so it can serve as documentation.
3. Document in readme (I'm happy to do that)

[sobstel]

Thanks a lot for quick and very detailed feedback. I found checking inclusions very tempting (mostly due to easy implementation) but no doubt, your suggestion is much more solid and bullet-proof. I actually had similar idea with expanding rules, but ruled it out at the beginning. I thought it might be a little too complex, but wrongly so. I'll let you know when I have it improved.

[sobstel]

1. On every rule definition, add an optional property polyfill property with the text that must go in the rule options.
2. In rule.js:create(), filter out the rules.

Both done. @robatwilliams Could you please have a look if we're on the same page regarding implementation details?

Usage:

{
      "
ecmascript-compat/compat": [
         "
error",
         {
           "
polyfills":[
                 "Array.prototype.flat",
                 "Array.prototype.flatMap",
                 "Array.prototype.includes",
                 "Promise.prototype.finally",
                 "Object.entries",
                 "Object.fromEntries",
                 "Object.getOwnPropertyDescriptors",
                 "Object.values",
                 "String.prototype.padStart",
                 "
String.prototype.replaceAll",
                 "
String.prototype.trimEnd",
                 "
String.prototype.trimStart",
              ],

Once past draft state, also: (...)

I'll add unit tests and update schema once we agree on above.

[robatwilliams]

Looks good, thanks.

I looked to see if I could allow the workflow to run without my approval, without changing the setting for all potential contributors - but there isn't anything. You can run it locally via npm run ci, or more specific tasks you'll find in package.json.

[sobstel]

You can run it locally via npm run ci (...)

It should pass now (at least it passes locally after most recent fixes).

[sobstel]

Thanks for updating docs @robatwilliams. I'll update the schema and unit tests.

[sobstel]

@robatwilliams I have added unit tests for all polyfills and updated json schema. It's ready for final review now.

> ci
> npm-run-all check test


> check
> npm-run-all --parallel check:*


> check:lint
> eslint --no-eslintrc --config=.eslintrc.json **/*.js


> check:prettier
> prettier --check **/*.{js,json,md}

Checking formatting...
All matched files use Prettier code style!

> test
> lerna run test

lerna notice cli v4.0.0
lerna info Executing command in 1 package: "npm run test"
lerna info run Ran npm script 'test' in 'eslint-plugin-ecmascript-compat' in 7.5s:

> eslint-plugin-ecmascript-compat@2.1.0 test
> jest --env=node

lerna success run Ran npm script 'test' in 1 package in 7.5s:
lerna success - eslint-plugin-ecmascript-compat

[robatwilliams]

Looks good, thanks. Just two straightfoward things.

As a final thing, could you try it out in the example subproject, with and without options given, just for one polyfill, to make sure it works outside of unit tests? Just run npm run lint there. No need to commit this.

[robatwilliams]

I looked at how eslint-plugin-compat (similar plugin for browser and language features, but not syntax) allows polyfills to be specified - https://github.com/amilajack/eslint-plugin-compat#adding-polyfills

That way allows sharing between different rules - https://eslint.org/docs/latest/user-guide/configuring/configuration-files#adding-shared-settings. But like us, they only have one rule.

I don't think we need to accept polyfills from this location instead of or in addition to the rule options. It's a global location and "polyfills" is a very generic key which different plugins might interpret differently. If a user wants both plugins to share config, they can define an array in their config file and use/spread it in both locations.

[sobstel]

As a final thing, could you try it out in the example subproject, (...)

Checked with random polyfills setup and it works ✅
Also, I've been running it with our code base all this time, so actually double-checked.

[robatwilliams]

Great, that big codebase is the best test.

[sobstel]

Thanks for approval. I cannot merge it on my own though. When do you think we can get it merged and released?

[robatwilliams]

Now released v2.2.0.

Thanks for your work, and please pass my thanks on to your team for continued contribution back to the plugin that you're making use of.