-
Notifications
You must be signed in to change notification settings - Fork 2
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
Convert to ESM #318
Convert to ESM #318
Conversation
import { chalk } from '@4c/cli-core/ConsoleUtilities'; | ||
import babel from '@babel/core'; | ||
import { safeLoad } from 'js-yaml'; | ||
import camelCase from 'lodash/camelCase.js'; |
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.
is there a good reason for these extensions? seems weird
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.
yes they are required for esm imports
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.
well not required for packages if the file is "main"
or "exports"
in the package.json sets it up. lodash doesn't set anything up
|
||
exports.builder = (_) => | ||
_.option('fix', { | ||
export function builder(_) { |
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.
what's up with this argument being an underscore?
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 preference
return execa('git', ['tag', '-d', tag]); | ||
} | ||
export function getCurrentBranch(opts) { | ||
return execa('git', ['rev-parse', '--abbrev-ref', 'HEAD'], opts).then( |
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.
any chance while we're touching this we can move from execa? might be better in a separate PR
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 don't want to move more things than necessary for this. i'm already pretty unsure i didn't break anything
I was tired of not being able to ever update of of sindresorhus's packages which all do native esm now