-
Notifications
You must be signed in to change notification settings - Fork 904
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
chore: use ESM exports where appropriate #151
chore: use ESM exports where appropriate #151
Conversation
@@ -191,7 +191,7 @@ async function setupAndRun() { | |||
} | |||
} | |||
|
|||
module.exports = { | |||
export default { |
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 wonder whether this shouldn't be export run
and export init
@@ -34,4 +34,4 @@ const withPods = { | |||
ios: ios.pod, | |||
}; | |||
|
|||
module.exports = { flat, nested, withExamples, withPods }; | |||
export default { flat, nested, withExamples, withPods }; |
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.
Shouldn't this be a series of named exports instead of an object?
@@ -43,7 +43,7 @@ function getDevices(): Array<string> { | |||
} | |||
} | |||
|
|||
module.exports = { | |||
export default { | |||
parseDevicesResult, |
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.
Shouldn't this be a set of named exports?
@@ -29,7 +29,7 @@ const debug = (...messages: Array<string>) => { | |||
console.log(`${chalk.black.bgWhite(' DEBUG ')} ${joinMessages(messages)}`); | |||
}; | |||
|
|||
module.exports = { | |||
export default { |
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.
Same here
@@ -52,7 +52,7 @@ function isGlobalCliUsingYarn(projectDir) { | |||
return fs.existsSync(path.join(projectDir, 'yarn.lock')); | |||
} | |||
|
|||
module.exports = { | |||
export default { |
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.
Same here
@@ -71,7 +71,7 @@ function remove(packageName, projectDir) { | |||
); | |||
} | |||
|
|||
module.exports = { | |||
export default { |
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.
Same here
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.
Final round, it looks good to me. I just wanted to suggest we refine some export default's when there's an object exported.
It makes sense to have export default {}
in case of a command where properties of an object are not meant to be consumed directly.
CC: @thymikee
@grabbou Ok, now I got it. Will add. |
Awesome @sidferreira! Thank you |
packages/cli/src/index.js
Outdated
@@ -17,4 +17,4 @@ if (require.main === module) { | |||
cli.run(); | |||
} | |||
|
|||
module.exports = cli; | |||
export default cli; |
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.
please revert that, this is public API
@grabbou the changes you requested ended being larger than expected. I reverted until we decide if we do the extra work now or wait for later. |
@sidferreira sounds good. Did you update the public API? |
@thymikee please update your review so we can land it! |
Thanks @sidferreira 👍 |
This is an extra for #150
Fixes exports and tweaks some tests.
The
link
,linkAssets
, andlinkDependency
where trickier as hell, but we have all tests working.I suggest take it easy in this review as I'm worried in a "false positive" in tests as there was a LOT of changes.
We need to improve the code coverage for changes like this have smaller chances of failing...