-
Notifications
You must be signed in to change notification settings - Fork 72
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
Require Node.js 12.20 and move to ESM #29
Conversation
BREAKING CHANGE: require Node.js >= v12.20
0e0e4f7
to
74e21de
Compare
cli.js
Outdated
}, {})); | ||
}; | ||
const normalizePluginOptions = plugin => | ||
// eslint-disable-next-line unicorn/no-array-reduce |
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.
Would be great if you could fix the lint instead of just ignoring it.
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.
I can replace it with any other type of loop, but imo reduce
is exactly what we need here — converting an array into a map.
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.
I would prefer a for-of
loop. I generally, find that to be more readable.
readme.md
Outdated
$ imagemin foo.png --plugin.webp.quality=95 --plugin.webp.preset=icon > foo-icon.webp | ||
``` | ||
|
||
**macOS** users can also use the short CLI syntax for array arguments: |
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.
I think non-Windows
would be more correct than macOS
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.
Just a typo. It should be macOS/linux
. But ok, non-Windows
is also fine.
cli.js
Outdated
import stripIndent from 'strip-indent'; | ||
import pairs from 'lodash.pairs'; | ||
|
||
const require = createRequire(import.meta.url); |
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.
require
should be replaced by await import(…)
.
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.
require('name')
searches for the module up the directory tree by diving into everynode_modules/<name>
. Аnd then it will also check the name
existence as global package. Dynamic import
checks just a single path.
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.
Dynamic import checks just a single path.
Are you sure about that? According to https://nodejs.org/api/esm.html#esm_resolver_algorithm_specification, static import
statement searches up, so I would have assumed dynamic import does the same.
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.
Wow! My experience bases on early polyfills. It's great that it just works now.
cli.js
Outdated
}, {})); | ||
}; | ||
const normalizePluginOptions = plugin => | ||
// eslint-disable-next-line unicorn/no-array-reduce |
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.
I would prefer a for-of
loop. I generally, find that to be more readable.
cli.js
Outdated
import stripIndent from 'strip-indent'; | ||
import pairs from 'lodash.pairs'; | ||
|
||
const require = createRequire(import.meta.url); |
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.
Dynamic import checks just a single path.
Are you sure about that? According to https://nodejs.org/api/esm.html#esm_resolver_algorithm_specification, static import
statement searches up, so I would have assumed dynamic import does the same.
Thanks :) |
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
meow
updatecloses docs(readme): add a note on how to pass arguments in [0,1] range #28
BREAKING CHANGE: require Node.js >= 12.20