Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement polyfills option in CLI #86

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions packages/check-es-compat/README.md
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are prettier formatting issues in this file - see CI

Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,16 @@ Firefox >= 58

<!--- Absolute link, in order to work from NPM website --->

The optional `--polyfills` argument can be used to specify polyfills that your application loads. These features are
therefore considered supported in all browsers. Features that are polyfillable and can be specified here can be found
in the [rule schema](https://github.com/robatwilliams/es-compat/blob/master/packages/eslint-plugin-ecmascript-compat/lib/rule.js).

```bash
$ npx check-es-compat . --polyfills="Array.prototype.includes,Promise.prototype.finally"
```

<!--- Absolute link, in order to work from NPM website --->

It [doesn't currently support](https://github.com/robatwilliams/es-compat/issues/69) ES modules.

<!--- Absolute link, in order to work from NPM website --->
Expand Down
43 changes: 41 additions & 2 deletions packages/check-es-compat/bin/cli.mjs
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated ESLint recently and it's now picked up some new issues. They seem reasonably, at a first glance.

Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
}
}

async function execute(files) {
async function execute(args) {

Check failure on line 20 in packages/check-es-compat/bin/cli.mjs

View workflow job for this annotation

GitHub Actions / ci (20.x)

'args' is already declared in the upper scope on line 5 column 7
const { files, polyfills } = parseArguments(args);
const eslint = new ESLint({
// Ignore any config files
useEslintrc: false,
Expand All @@ -34,7 +35,7 @@
es2023: true,
},
rules: {
'ecmascript-compat/compat': 'error',
'ecmascript-compat/compat': ['error', { polyfills }],
},
},
});
Expand All @@ -46,3 +47,41 @@

return { hasErrors: results.some((result) => result.errorCount > 0) };
}

function parseArguments(args) {

Check failure on line 51 in packages/check-es-compat/bin/cli.mjs

View workflow job for this annotation

GitHub Actions / ci (20.x)

Function 'parseArguments' has too many statements (15). Maximum allowed is 10

Check failure on line 51 in packages/check-es-compat/bin/cli.mjs

View workflow job for this annotation

GitHub Actions / ci (20.x)

'args' is already declared in the upper scope on line 5 column 7
NoelDeMartin marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hesitant to put this code in - there are a few different branches and checks so it's not straightforward to have confidence whether it covers everything it needs to.

I think it should use minimist.

Also there are likely to be many polyfills so perhaps it would be better instead to take a --polyfills-file that is a path to a file with one polyfill on each line.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, ideally it should use minimist, but I decided not to do it because that would likely be a lot more work. And I'm not sure you'd like the idea to add new dependencies, etc.

In any case, I implemented this for our project and I thought I'd contribute it rather than just opening an issue. If you think this is not the right direction for the library, I think it's better if you close this PR and work on this yourself (or I can open a normal issue to keep track of the missing feature). But I don't think I'll take it further, because this simple approach is enough for our use-case and we're using the patch with patch-package.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, thanks. I've created issue #93 for it, but am not planning to work on it.

const files = [];
const polyfills = [];
let nextArgIsPolyfills = false;

for (const arg of args) {
if (nextArgIsPolyfills) {
nextArgIsPolyfills = false;
polyfills.push(...splitPolyfillsArgument(arg));
continue;

Check failure on line 60 in packages/check-es-compat/bin/cli.mjs

View workflow job for this annotation

GitHub Actions / ci (20.x)

Unexpected use of continue statement
}

if (arg.startsWith('--polyfills')) {
if (arg.startsWith('--polyfills=')) {
polyfills.push(...splitPolyfillsArgument(arg.slice(12)));
} else {
nextArgIsPolyfills = true;
}

continue;

Check failure on line 70 in packages/check-es-compat/bin/cli.mjs

View workflow job for this annotation

GitHub Actions / ci (20.x)

Unexpected use of continue statement
}

files.push(arg);
}

return { files, polyfills };
}

function splitPolyfillsArgument(polyfills) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very specific, and in future (in #90 for ES2023 actually) would need updating if we add similar polyfills.

Another reason to use a file for passing in the polyfills list?

const prototypeAtPolyfill = '{Array,String,TypedArray}.prototype.at';
const prototypeAtPlaceholder = '{{PROTOTYPEAT}}';

return polyfills
.replace(prototypeAtPolyfill, prototypeAtPlaceholder)
.split(',')
.map(polyfill => polyfill === prototypeAtPlaceholder ? prototypeAtPolyfill : polyfill);
}
Loading