Skip to content

chore: use ESM exports where appropriate#151

Merged
thymikee merged 9 commits intoreact-native-community:masterfrom
sidferreira:fixExports
Feb 7, 2019
Merged

chore: use ESM exports where appropriate#151
thymikee merged 9 commits intoreact-native-community:masterfrom
sidferreira:fixExports

Conversation

@sidferreira
Copy link

This is an extra for #150
Fixes exports and tweaks some tests.

The link, linkAssets, and linkDependency 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...

@sidferreira
Copy link
Author

@grabbou

}

module.exports = {
export default {
Copy link
Member

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

};

module.exports = { flat, nested, withExamples, withPods };
export default { flat, nested, withExamples, withPods };
Copy link
Member

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?


module.exports = {
export default {
parseDevicesResult,
Copy link
Member

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?

};

module.exports = {
export default {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

}

module.exports = {
export default {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

}

module.exports = {
export default {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Member

@grabbou grabbou left a 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

@sidferreira
Copy link
Author

sidferreira commented Feb 6, 2019

@grabbou Ok, now I got it. Will add.

@grabbou
Copy link
Member

grabbou commented Feb 6, 2019

Awesome @sidferreira! Thank you

}

module.exports = cli;
export default cli;
Copy link
Member

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

@sidferreira
Copy link
Author

@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.

@grabbou
Copy link
Member

grabbou commented Feb 7, 2019

@sidferreira sounds good. Did you update the public API?

@grabbou
Copy link
Member

grabbou commented Feb 7, 2019

@thymikee please update your review so we can land it! :shipit:

@thymikee thymikee changed the title Fix exports chore: use ESM exports where appropriate Feb 7, 2019
@thymikee thymikee merged commit fb4c399 into react-native-community:master Feb 7, 2019
@thymikee
Copy link
Member

thymikee commented Feb 7, 2019

Thanks @sidferreira 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants