-
Notifications
You must be signed in to change notification settings - Fork 14
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -17,7 +17,8 @@ | |
} | ||
} | ||
|
||
async function execute(files) { | ||
async function execute(args) { | ||
const { files, polyfills } = parseArguments(args); | ||
const eslint = new ESLint({ | ||
// Ignore any config files | ||
useEslintrc: false, | ||
|
@@ -34,7 +35,7 @@ | |
es2023: true, | ||
}, | ||
rules: { | ||
'ecmascript-compat/compat': 'error', | ||
'ecmascript-compat/compat': ['error', { polyfills }], | ||
}, | ||
}, | ||
}); | ||
|
@@ -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 GitHub Actions / ci (20.x)
|
||
NoelDeMartin marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
if (arg.startsWith('--polyfills')) { | ||
if (arg.startsWith('--polyfills=')) { | ||
polyfills.push(...splitPolyfillsArgument(arg.slice(12))); | ||
} else { | ||
nextArgIsPolyfills = true; | ||
} | ||
|
||
continue; | ||
} | ||
|
||
files.push(arg); | ||
} | ||
|
||
return { files, polyfills }; | ||
} | ||
|
||
function splitPolyfillsArgument(polyfills) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} |
There was a problem hiding this comment.
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