Standardize imports

[marcaaron]

Details

Fixed Issues

$ #6072

Tests

• Make sure the app builds OK and there are no obvious issues

QA Steps

• None other than regression testing. This touches a lot of code so it is not realistic to test specific areas of the app. However, there are no changes in behavior so we should just be on the look out for unexpected changes.

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

[aldo-expensify]

Should we add exceptions for very specific cases? Or perhaps we should just update the usages even if the linter is not complaining? The goal is to get people to mostly do something consistently - my feeling is that even if it's not yet perfect it is better than it was 😄

Yep, had the same doubt, I leave it totally up to you. If I was working on it, I would add it because it is just one, but if we had many (hopefully won't happen) it could become a pain to maintain and I wouldn't do it.

[aldo-expensify]

I think no, but maybe it could be improved on. All our styles live in styles/styles.js with one big default export of all styles and then named exports for the "style helpers". Probably a better way to do this would be to move all the helper methods into something like StyleUtils.js and then import * as StyleUtils from './style/StyleUtils or something like that.

Moving the methods to a new StyleUtils.js sounds like a good way to organize these functions.

I asked this question because I saw some cases where we are still destructuring imports from styles and I don't know if they were intentionally left like that, i.e.:

App/src/components/TextInputAutoWidth.js

Line 5 in 0aa9834

import styles, {getAutoGrowTextInputStyle} from '../styles/styles';

App/src/components/ThumbnailImage.js

Line 7 in 0aa9834

import styles, {getWidthAndHeightStyle} from '../styles/styles';

[marcaaron]

I asked this question because I saw some cases where we are still destructuring imports from styles and I don't know if they were intentionally left like that, i.e.:

Yeah maybe this is a bit confusing, but the new guidance is to only warn if we are not mixing default and named together in one import so those examples you shared are actually OK but something like this would not be OK...

 import {getAutoGrowTextInputStyle} from '../styles/styles'; 

[marcaaron]

It gets confusing when you are not mixing these together e.g. how should we fix that previous example?

import * as StyleUtils from '../styles/styles'; 

Doesn't seem quite right if there's still a default export. Gonna take some time to fix these up since @jasperhuangg had a similar question and this will probably remain confusing. I think the long term mission is to stop mixing these together and just use one or the other.

[marcaaron]

Ok, I think we are good now once e2e finishes 🤞

[aldo-expensify]

I asked this question because I saw some cases where we are still destructuring imports from styles and I don't know if they were intentionally left like that, i.e.:

Yeah maybe this is a bit confusing, but the new guidance is to only warn if we are not mixing default and named together in one import so those examples you shared are actually OK but something like this would not be OK...

 import {getAutoGrowTextInputStyle} from '../styles/styles'; 

I see, thanks for explaining

[aldo-expensify]

I think your have to update the package-lock.json

[aldo-expensify]

I think this needs to be updated. If I try to do npm install with the committed package-lock.json, I get this error:

npm ERR! code 128
npm ERR! Command failed: git checkout afc71e84361cc68af4a85b425b3a599c4ac1861d
npm ERR! fatal: reference is not a tree: afc71e84361cc68af4a85b425b3a599c4ac1861d
npm ERR! 

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/aldocanepa/.npm/_logs/2021-11-24T18_57_59_383Z-debug.log

Deleting the package-lock.json and running npm install changes to:

"eslint-config-expensify": {
    "version": "2.0.19",
    "resolved": "git+https://github.com/Expensify/eslint-config-expensify.git#e1edb2cfe7fd91b9b131187cb19481ba9d29705b",
    ...
}

[marcaaron]

Oh hmm weird..

[marcaaron]

@aldo-expensify let me know if that solved it for ya!

[aldo-expensify]

@aldo-expensify let me know if that solved it for ya!

It did 👍

[aldo-expensify]

Looks good to me! feel free to merge once the e2e tests pass!

[botify]

@marcaaron looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[marcaaron]

Tbh I don't know why the Emergency label was added - it looks like the tests passed to me. If there was a test still going it must have been the E2E and I missed that it was running. But there's no Emergency here.

[marcaaron]

[OSBotify]

🚀 Deployed to staging by @marcaaron in version: 1.1.16-11 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production by @roryabraham in version: 1.1.17-7 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅