-
Notifications
You must be signed in to change notification settings - Fork 45.8k
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
Simplify imports in react reconciler #13718
Conversation
Details of bundled changes.Comparing: ae4f3f0...da6419a scheduler
Generated by 🚫 dangerJS |
…ify_imports_in_react-reconciler
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.
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'; |
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'm curious if this was here for a reason. @gaearon do you know of any reason why this can't be relative?
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.
Yes, it was because Flow has issues with symlinks on Windows.
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.
That said we didn't follow it consistently either.
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.
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.
@nhunzaker Merge master here. If there are something wrong with it, let me know |
Sorry! Ran the build, no change in build size. This looks good to go 👍. Thanks! |
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. |
@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? |
Let's keep it this way I guess since so far it doesn't fail. Yeah I know it's not clear. No worries. |
* 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
Refactoring and implementing one code style for imports in react-reconciler package