Implement polyfills option in CLI #86
Conversation
NoelDeMartin
force-pushed
the
cli-polyfills
branch
2 times, most recently
from
6e1de62
to
8d0affa
Compare
The check-es-compat patch can be removed if the following PR is resolved: robatwilliams/es-compat#86
The check-es-compat patch can be removed if the following PR is resolved: robatwilliams/es-compat#86
|
Hi, thanks for this. Just to say I've seen it and I will get round to taking a proper look. |
robatwilliams
force-pushed
the
cli-polyfills
branch
from
8d0affa
to
33392cd
Compare
There was a problem hiding this comment.
There are prettier formatting issues in this file - see CI
There was a problem hiding this comment.
I've updated ESLint recently and it's now picked up some new issues. They seem reasonably, at a first glance.
| @@ -46,3 +47,41 @@ async function execute(files) { | |||
|
|
|||
| return { hasErrors: results.some((result) => result.errorCount > 0) }; | |||
| } | |||
|
|
|||
| function parseArguments(args) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Fair, thanks. I've created issue #93 for it, but am not planning to work on it.
| return { files, polyfills }; | ||
| } | ||
|
|
||
| function splitPolyfillsArgument(polyfills) { |
There was a problem hiding this comment.
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?
The check-es-compat patch can be removed if the following PR is resolved: robatwilliams/es-compat#86
This should make it possible to use the functionality introduced in #50 with the CLI.
I'm aware that this implementation is a bit naive in the parsing and such, but I thought it would be overkill to add a new dependency to convert this simple script into a "proper CLI" (something like minimist for example).