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

Simplify imports in react reconciler #13718

Merged

Conversation

mmarkelov
Copy link
Contributor

Refactoring and implementing one code style for imports in react-reconciler package

@sizebot
Copy link

sizebot commented Sep 24, 2018

Details of bundled changes.

Comparing: ae4f3f0...da6419a

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

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

Awesome. We usually don't merge stylistic changes, but this feels like a general improvement.

Do you mind resolving conflicts with master?

import type {Fiber} from 'react-reconciler/src/ReactFiber';
import type {ExpirationTime} from 'react-reconciler/src/ReactFiberExpirationTime';
import type {Fiber} from './ReactFiber';
import type {ExpirationTime} from './ReactFiberExpirationTime';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious if this was here for a reason. @gaearon do you know of any reason why this can't be relative?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it was because Flow has issues with symlinks on Windows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That said we didn't follow it consistently either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Reason enough to revert this? There's still the de-duplication stuff in here. We could revise this PR to remove the relative import changes.

@mmarkelov
Copy link
Contributor Author

@nhunzaker Merge master here. If there are something wrong with it, let me know

@nhunzaker
Copy link
Contributor

Sorry!

Ran the build, no change in build size. This looks good to go 👍.

Thanks!

@nhunzaker nhunzaker merged commit d5d10d1 into facebook:master Nov 1, 2018
@gaearon
Copy link
Collaborator

gaearon commented Nov 1, 2018

The type imports were intentionally "absolute" to avoid symlink resolution issues on Windows.

This also broke CI after merge. (Maybe because some other change happened on master?)

Let's avoid stylistic changes like this indeed if we're not pursuing some larger goal.

@mmarkelov
Copy link
Contributor Author

mmarkelov commented Nov 1, 2018

@gaearon, @nhunzaker I can do pr to get it back. And sorry, but it was absolutely unclear that it will get issue in Windows. Or it is better to revent?

@gaearon
Copy link
Collaborator

gaearon commented Nov 1, 2018

Let's keep it this way I guess since so far it doesn't fail. Yeah I know it's not clear. No worries.

jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
* Simplify imports in ReactChildFiber
* Import type first in ReactCurrentFiber
* Simplify imports in ReactFiberBeginWork
* Simplify imports in ReactFiberScheduler
* Simplify import in ReactFiberTreeReflection
* Simplify import in ReactFiberUnwindWork
* Remove repeated import
* Fix imports from ReactFiberExpirationTime
* Master imports in ReactFiberBeginWork
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants