-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[No QA] Sorted import aliases #26605
Conversation
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 not sure, but I think we can do. |
Yes, I tested and posted the video in the issue as well. I have tried everything that required for this PR. |
Tested again and works now. Not sure why it didn't work first time. |
What's your plan to apply changes to all files? Manual? |
It surely requires a restart for server. |
maybe we can try with eslint-plugin-import-alias package |
But that was way too old, not maintained. I suggest we don't go with it. I'll check for some other alternatives and will update. |
@roryabraham what's your suggestion on #26605 (comment)? |
I agree https://www.npmjs.com/package/eslint-plugin-import-alias hasn't been updated in a long time, but let's see if it works for us. It might have just been stable to the point where it doesn't need any updates since it seems a pretty simple lint rule of a very stable syntax (ES6 relative and aliased imports). If it doesn't work or we uncover some problem with it, then we can re-evaluate. |
Maybe this works -> https://github.com/dword-design/eslint-plugin-import-alias |
Sure, can you try with that package? https://www.npmjs.com/package/@dword-design/eslint-plugin-import-alias |
@b4s36t4 Any progress? |
Hey, @situchan not yet. Been stuck at some work. Will update the progress by night. |
@situchan @roryabraham The above package is working perfect. I was able to update all imports with After imports there are 9 errors which are related sort-order (I feel this is not an issue, can raise new PR for solving these). |
Great!
Though it doesn't affect github workflow right? Lint will still success, correct? @roryabraham what is the plan to merge first PR? At what time? |
No lint will fail, @situchan |
We can't merge PR with lint failure |
|
In this case, we can do both import alias and fix import order in one PR. @roryabraham what are your thoughts? |
Yes but that might raise merge conflict (not many though) we should let people know before merging. |
That should happen even when we separate PRs |
@roryabraham What should we do about the above concerns? |
Agree with @situchan – we can't merge a PR with lint failing (or any warnings either). We need to manually fix the 9 sort-import warnings in this PR.
Can we try to add https://github.com/trivago/prettier-plugin-sort-imports or something similar so that Prettier can automatically fix the import sort order?
You mean we should let people know after merging 😈 but yes, after merging this PR I will announce in #expensify-open-source that everyone needs to merge main into their PR branches. |
Let's make sure it doesn't affect eslint performance, source |
@situchan Yes, it will fail (npm run lint). |
I'm back from OOO so let's get this cleaned up again then take it over the finish line |
a83610e
to
6fd7e62
Compare
Reviewer Checklist
Screenshots/Videos |
Just conflict arrived. Let's quickly fix |
df244b8
to
5d45f0f
Compare
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
It seems very unlikely that this compile-time change has any impact on app performance |
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.93-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.93-1 🚀
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.94-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.94-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.94-2 🚀
|
Details
Fixed Issues
$ #20531
PROPOSAL: #20531 (comment)
Tests
NA
Offline tests
NA
QA Steps
NA
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Kapture.2023-10-09.at.17.18.09.mp4
Mobile Web - Chrome
Mobile Web - Safari
Kapture.2023-10-09.at.17.17.10.mp4
Desktop
Kapture.2023-10-09.at.17.54.19.mp4
iOS
Kapture.2023-10-09.at.17.15.53.mp4
Android