-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix: apply eslint suggestions #111
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.
Interesting! The linter modified some upstream files last time.
Looks like Chrome ran the linter on their end too and the file doesn't have those anymore.
rn-chrome-devtools-frontend/front_end/panels/js_profiler/js_profiler-meta.ts
Lines 31 to 39 in a556d26
record: 'Record', | |
/** | |
*@description Text of an item that stops the running task | |
*/ | |
stop: 'Stop', | |
/** | |
*@description Title of an action in the timeline tool to record reload | |
*/ | |
startProfilingAndReloadPage: 'Start profiling and reload page', |
@@ -247,8 +247,8 @@ class FuseboxReactNativeApplicationObserver implements | |||
const {appDisplayName, deviceName} = event.data; | |||
|
|||
// Update window title | |||
if (appDisplayName != null) { | |||
document.title = `${appDisplayName}${deviceName != null ? ` (${deviceName})` : ''} - React Native DevTools`; | |||
if (appDisplayName !== null && appDisplayName !== undefined) { |
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.
Bit unfortunate about this check — != null
is FB code style and more concise. Happy to align but we could consider overriding for META_CODE_PATHS
.
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 am leaning more towards adopting what is already used in the project. Like what if, for example, we need to change non-FB codepaths? For hiding unsupported features, should we use FB code style or not?
I will keep it as it is now, but open to discuss the whole approach in a broader group, since we already have a list of things: #
vs private
, !=
vs !==
, ...
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 believe the existing repo convention is to use if (object)
or if (Boolean(…))
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've changed it this way, so there are no functional changes
e. g. a != null
is the same as a !== null && a !== undefined
, but not the same as !!a
or Boolean(a)
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 the meaning would not the same for this specific line. I'm partial to adopting to local patterns too, so the if (object)
or if (Boolean(…))
seems to be the general convention here.
Tho in this specific case, it'd would seem like if (appDisplayName)
would work slightly better as it'd filter out (already-trimmed) empty strings as well.
I'm totally happy that this PR as-is is scoped strictly on logical parity (same functional outcomes before and after lint fixes). Items mentioned in this thread is for future considerations when/if we do have a wider conversation around the style
Summary
Stacked on #110. See this commit.
Test plan
npm run check-lint
no longer produces errors for our custom files.Upstreaming plan
devtools-frontend
repo. I've reviewed the contribution guide.