Skip to content
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

[WIP] Change require to imports (alt) #150

Conversation

sidferreira
Copy link

No description provided.

@thymikee
Copy link
Member

thymikee commented Feb 4, 2019

Mind fixing conflicts and address feedback I left on #145?

@sidferreira sidferreira force-pushed the refactor/replacesRequiresByImports branch from 781c6ce to d966ec4 Compare February 4, 2019 15:51
@sidferreira sidferreira force-pushed the refactor/replacesRequiresByImports branch from d966ec4 to 538f651 Compare February 4, 2019 16:07
packages/cli/src/index.js Outdated Show resolved Hide resolved
@thymikee
Copy link
Member

thymikee commented Feb 4, 2019

@Trancever mind checking that?

packages/cli/src/index.js Show resolved Hide resolved
packages/cli/src/server/util/messageSocket.js Show resolved Hide resolved
packages/global-cli/index.js Outdated Show resolved Hide resolved
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 so much for helping us out, it's greatly appreciated 👍

@sidferreira
Copy link
Author

@thymikee I hope to help even more in the future 👍

@grabbou
Copy link
Member

grabbou commented Feb 4, 2019

@sidferreira amazing work, really impressive! Looks like module.exports are still there - is the intention to migrate to export syntax at some point, presumably in next PR?

@sidferreira
Copy link
Author

sidferreira commented Feb 4, 2019

@grabbou as it isn't merged yet, I can take a look at it now

Change the exports will be a little trickier, as it requires changes at the tests too.

Also glob seem to have issues with import so it will need extra time to change and test it properly

edit: Tried a bit more and it will need a lot of (not hard, just slow) work to tweak the tests. ATM there are 2 errors related to link function

@grabbou
Copy link
Member

grabbou commented Feb 5, 2019

No worries, we can do it after we merge this one. Shall we land it?

@thymikee
Copy link
Member

thymikee commented Feb 5, 2019

If you don't have anything to add then feel free to press the button!

@grabbou grabbou merged commit ef220e2 into react-native-community:master Feb 5, 2019
grabbou added a commit that referenced this pull request Feb 5, 2019
grabbou pushed a commit that referenced this pull request Feb 5, 2019
* refactor(imports): replaces most uses of require by import

* fix: fixes export and revert wrong change

* fix: fix import order

* fix: revert export change

* fix: revert global-cli/index

* fix: typo
@grabbou
Copy link
Member

grabbou commented Feb 5, 2019

Had to force push as I forgot to reword the PR title.

@sidferreira sidferreira deleted the refactor/replacesRequiresByImports branch February 6, 2019 08:07
thymikee added a commit that referenced this pull request Feb 6, 2019
Summary:
---------

After #150 (thanks @sidferreira and @Trancever) we're finally able to use lazy module evaluation for additional perf! 

Before:
```
0.70 real         0.64 user         0.12 sys
```
After:
```
0.19 real         0.16 user         0.03 sys
```

Which gives us roughly 0.5s (~72%) improvement in startup time. With 0.2s startup time most commands feel instant now.

Test Plan:
----------

No new tests added.
thymikee pushed a commit that referenced this pull request Feb 7, 2019
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...
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