Skip to content

[WIP] Change require to imports #145

Closed
Trancever wants to merge 3 commits intoreact-native-community:masterfrom
Trancever:refactor/require-to-import
Closed

[WIP] Change require to imports #145
Trancever wants to merge 3 commits intoreact-native-community:masterfrom
Trancever:refactor/require-to-import

Conversation

@Trancever
Copy link

Summary:

Fix for #114

Test Plan:

This is still work in progress since many tests are failing.

@Trancever Trancever changed the title [WIP] Change requires to imports [WIP] Change require to imports Feb 4, 2019
Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @Trancever! Apart from fixing tests, there's still a bunch of issues to address. But grat work overall :)


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

Choose a reason for hiding this comment

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

later on we can think about using named exports instead of this

import childProcess from 'child_process';
import path from 'path';
import chalk from 'chalk';
import helpers from './helpers';
Copy link
Member

Choose a reason for hiding this comment

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

we run scripts through regular node, without ESM loader, so please revert that as it won't work now

import child_process from 'child_process';
import chalk from 'chalk';
import prompt from 'prompt';
import semver from 'semver';
Copy link
Member

Choose a reason for hiding this comment

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

global-cli is not compiled with Babel, please revert changes here for now.

Copy link
Member

Choose a reason for hiding this comment

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

And it's going to be deprecated anyway, so I'll just skip it.

}

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

@thymikee thymikee Feb 4, 2019

Choose a reason for hiding this comment

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

please revert this export change as it breaks our public API for those using require syntax.
Instead of doing:

const rncli = require('@react-native-community/cli');

They would have to do:

const rncli = require('@react-native-community/cli').default;

because of Babel interop. The API however won't change for folks using import syntax, because it has this check built-in (both in Babel and TS afaik)

@Trancever
Copy link
Author

Thanks @thymikee for review. Will try to fix those issues as soon as possible

@sidferreira
Copy link

@Trancever Let's compare our branches. My tests are all passing...

@Trancever
Copy link
Author

@sidferreira Nice! Can you open PR with your changes? I think we should proceed with your version if you have it already working.

@sidferreira
Copy link

Added as #150

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.

4 participants