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.

Sign up for GitHub

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

Jump to bottom

fix: apply eslint suggestions #111

Merged
merged 2 commits into from
Sep 9, 2024
Merged

fix: apply eslint suggestions #111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b8132fc
fix[react-devtools]: initialize bridge only after domain is ready
hoxyq Sep 5, 2024
ae70adc
fix: apply eslint suggestions
hoxyq Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions front_end/entrypoints/rn_fusebox/rn_fusebox.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link

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.

Copy link
Author

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 !==, ...

Copy link

@EdmondChuiHW EdmondChuiHW Sep 12, 2024 •
edited
Loading

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(…))

Copy link
Author

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)

Copy link

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

document.title = `${appDisplayName}${deviceName !== null && deviceName !== undefined ? ` (${deviceName})` : ''} - React Native DevTools`;
}
}
}
Expand Down
32 changes: 18 additions & 14 deletions front_end/panels/react_devtools/ReactDevToolsModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ export class ReactDevToolsModel extends SDK.SDKModel.SDKModel<EventTypes> {
readonly #listeners: Set<ReactDevToolsTypes.WallListener> = new Set();
#initializeCalled: boolean = false;
#initialized: boolean = false;
#bridge: ReactDevToolsTypes.Bridge | null;
#store: ReactDevToolsTypes.Store | null;
#bridge: ReactDevToolsTypes.Bridge | null = null;
#store: ReactDevToolsTypes.Store | null = null;

constructor(target: SDK.Target.Target) {
super(target);
Expand All @@ -52,11 +52,9 @@ export class ReactDevToolsModel extends SDK.SDKModel.SDKModel<EventTypes> {
},
send: (event, payload): void => void this.#sendMessage({event, payload}),
};
this.#bridge = ReactDevTools.createBridge(this.#wall);
this.#store = ReactDevTools.createStore(this.#bridge);

const bindingsModel = target.model(ReactNativeModels.ReactDevToolsBindingsModel.ReactDevToolsBindingsModel);
if (bindingsModel == null) {
if (bindingsModel === null) {
throw new Error('Failed to construct ReactDevToolsModel: ReactDevToolsBindingsModel was null');
}

Expand All @@ -82,13 +80,16 @@ export class ReactDevToolsModel extends SDK.SDKModel.SDKModel<EventTypes> {
window.addEventListener('beforeunload', () => this.#bridge?.shutdown());
}

async ensureInitialized(): Promise<void> {
ensureInitialized(): void {
if (this.#initializeCalled) {
return;
}

this.#initializeCalled = true;
void this.#initialize();
}

async #initialize(): Promise<void> {
try {
const bindingsModel = this.#bindingsModel;
await bindingsModel.enable();
Expand All @@ -101,7 +102,7 @@ export class ReactDevToolsModel extends SDK.SDKModel.SDKModel<EventTypes> {
await bindingsModel.initializeDomain(ReactDevToolsModel.FUSEBOX_BINDING_NAMESPACE);

this.#initialized = true;
this.dispatchEventToListeners(Events.InitializationCompleted);
this.#finishInitializationAndNotify();
} catch (e) {
this.dispatchEventToListeners(Events.InitializationFailed, e.message);
}
Expand All @@ -112,15 +113,15 @@ export class ReactDevToolsModel extends SDK.SDKModel.SDKModel<EventTypes> {
}

getBridgeOrThrow(): ReactDevToolsTypes.Bridge {
if (this.#bridge == null) {
if (this.#bridge === null) {
throw new Error('Failed to get bridge from ReactDevToolsModel: bridge was null');
}

return this.#bridge;
}

getStoreOrThrow(): ReactDevToolsTypes.Store {
if (this.#store == null) {
if (this.#store === null) {
throw new Error('Failed to get store from ReactDevToolsModel: store was null');
}

Expand Down Expand Up @@ -152,17 +153,20 @@ export class ReactDevToolsModel extends SDK.SDKModel.SDKModel<EventTypes> {
throw new Error('ReactDevToolsModel failed to handle BackendExecutionContextCreated event: ReactDevToolsBindingsModel was null');
}

this.#bridge = ReactDevTools.createBridge(this.#wall);
this.#store = ReactDevTools.createStore(this.#bridge);

// This could happen if the app was reloaded while ReactDevToolsBindingsModel was initializing
if (!rdtBindingsModel.isEnabled()) {
void this.ensureInitialized();
this.ensureInitialized();
} else {
this.dispatchEventToListeners(Events.InitializationCompleted);
this.#finishInitializationAndNotify();
}
}

#finishInitializationAndNotify(): void {
this.#bridge = ReactDevTools.createBridge(this.#wall);
this.#store = ReactDevTools.createStore(this.#bridge);
this.dispatchEventToListeners(Events.InitializationCompleted);
}

#handleBackendExecutionContextUnavailable({data: errorMessage}: ReactDevToolsBindingsBackendExecutionContextUnavailableEvent): void {
this.dispatchEventToListeners(Events.InitializationFailed, errorMessage);
}
Expand Down
27 changes: 16 additions & 11 deletions front_end/panels/react_devtools/ReactDevToolsViewBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {Events as ReactDevToolsModelEvents, ReactDevToolsModel, type EventTypes

import type * as ReactDevToolsTypes from '../../third_party/react-devtools/react-devtools.js';
import type * as Platform from '../../core/platform/platform.js';
import { LocalizedString } from '../../core/platform/UIString.js';

const UIStrings = {
/**
Expand Down Expand Up @@ -76,7 +75,7 @@ export class ReactDevToolsViewBase extends UI.View.SimpleView implements

constructor(
tab: 'components' | 'profiler',
title: LocalizedString,
title: Platform.UIString.LocalizedString,
) {
super(title);

Expand All @@ -88,20 +87,14 @@ export class ReactDevToolsViewBase extends UI.View.SimpleView implements
super.wasShown();
this.registerCSSFiles([ReactDevTools.CSS]);

if (this.#model == null) {
if (this.#model === null) {
SDK.TargetManager.TargetManager.instance().observeModels(ReactDevToolsModel, this);
}
}

modelAdded(model: ReactDevToolsModel): void {
this.#model = model;

if (model.isInitialized()) {
// Already initialized from another rendered React DevTools view - render
// from initialized state
this.#renderDevToolsView();
}

model.addEventListener(
ReactDevToolsModelEvents.InitializationCompleted,
this.#handleInitializationCompleted,
Expand All @@ -117,7 +110,15 @@ export class ReactDevToolsViewBase extends UI.View.SimpleView implements
this.#handleBackendDestroyed,
this,
);
void model.ensureInitialized();

if (model.isInitialized()) {
// Already initialized from another rendered React DevTools panel - render
// from initialized state
this.#renderDevToolsView();
} else {
// Once initialized, it will emit InitializationCompleted event
model.ensureInitialized();
}
}

modelRemoved(model: ReactDevToolsModel): void {
Expand Down Expand Up @@ -153,7 +154,11 @@ export class ReactDevToolsViewBase extends UI.View.SimpleView implements
#renderDevToolsView(): void {
this.#clearView();

const model = this.#model!;
const model = this.#model;
if (model === null) {
throw new Error('Attempted to render React DevTools panel, but the model was null');
}

const usingDarkTheme = window.matchMedia('(prefers-color-scheme: dark)').matches;
const initializeFn = this.#tab === 'components' ? ReactDevTools.initializeComponents : ReactDevTools.initializeProfiler;

Expand Down
20 changes: 8 additions & 12 deletions front_end/panels/rn_welcome/RNWelcome.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,6 @@ const UIStrings = {
docsReactDevTools: 'React DevTools',
/** @description "React DevTools" item detail */
docsReactDevToolsDetail: 'Debug React components with React DevTools',
/** @description "React Native DevTools" title (docs item 2 - post launch) */
docsRNDevTools: 'React Native DevTools',
/** @description "React Native DevTools" item detail */
docsRNDevToolsDetail: 'Explore features available in React Native DevTools',
/** @description "Native Debugging" title (docs item 3) */
docsNativeDebugging: 'Native Debugging',
/** @description "Native Debugging" item detail */
Expand All @@ -53,7 +49,7 @@ type RNWelcomeOptions = {
debuggerBrandName: () => Platform.UIString.LocalizedString,
showBetaLabel?: boolean,
showTechPreviewLabel?: boolean,
showDocs?: boolean
showDocs?: boolean,
};

export class RNWelcomeImpl extends UI.Widget.VBox implements
Expand Down Expand Up @@ -106,7 +102,7 @@ export class RNWelcomeImpl extends UI.Widget.VBox implements
}
}

private _handleLinkPress(url: string): void {
#handleLinkPress(url: string): void {
Host.InspectorFrontendHost.InspectorFrontendHostInstance.openInNewTab(
url as Platform.DevToolsPath.UrlString,
);
Expand All @@ -117,7 +113,7 @@ export class RNWelcomeImpl extends UI.Widget.VBox implements
debuggerBrandName,
showBetaLabel = false,
showTechPreviewLabel = false,
showDocs = false
showDocs = false,
} = this.options;
const welcomeIconUrl = new URL(
'../../Images/react_native/welcomeIcon.png',
Expand All @@ -140,7 +136,7 @@ export class RNWelcomeImpl extends UI.Widget.VBox implements
<div class="rn-welcome-panel">
<header class="rn-welcome-hero">
<div class="rn-welcome-heading">
<img class="rn-welcome-icon" src="${welcomeIconUrl}" role="presentation" />
<img class="rn-welcome-icon" src=${welcomeIconUrl} role="presentation" />
<h1 class="rn-welcome-title">
${debuggerBrandName()}
</h1>
Expand All @@ -166,29 +162,29 @@ export class RNWelcomeImpl extends UI.Widget.VBox implements
${i18nString(UIStrings.whatsNewLabel)}
</x-link>
</div>
${this.#reactNativeVersion != null ? html`
${this.#reactNativeVersion !== null && this.#reactNativeVersion !== undefined ? html`
<p class="rn-welcome-version">React Native: <code>${this.#reactNativeVersion}</code></p>
` : null}
</header>
${showDocs ? html`
<section class="rn-welcome-docsfeed">
<h2 class="rn-welcome-h2">Learn</h2>
<button class="rn-welcome-docsfeed-item" type="button" role="link" @click=${this._handleLinkPress.bind(this, 'https:\/\/reactnative.dev/docs/debugging')} title="${i18nString(UIStrings.docsDebuggingBasics)}">
<button class="rn-welcome-docsfeed-item" type="button" role="link" @click=${this.#handleLinkPress.bind(this, 'https:\/\/reactnative.dev/docs/debugging')} title=${i18nString(UIStrings.docsDebuggingBasics)}>
<div class="rn-welcome-image" style="background-image: url('${docsImage1Url}')"></div>
<div>
<p class="devtools-link">${i18nString(UIStrings.docsDebuggingBasics)}</p>
<p>${i18nString(UIStrings.docsDebuggingBasicsDetail)}</p>
</div>
</button>
<!-- TODO(huntie): Replace this item when React Native DevTools docs are complete -->
<button class="rn-welcome-docsfeed-item" type="button" role="link" @click=${this._handleLinkPress.bind(this, 'https:\/\/reactnative.dev/docs/debugging/react-devtools')} title="${i18nString(UIStrings.docsReactDevTools)}">
<button class="rn-welcome-docsfeed-item" type="button" role="link" @click=${this.#handleLinkPress.bind(this, 'https:\/\/reactnative.dev/docs/debugging/react-devtools')} title=${i18nString(UIStrings.docsReactDevTools)}>
<div class="rn-welcome-image" style="background-image: url('${docsImage2Url}')"></div>
<div>
<p class="devtools-link">${i18nString(UIStrings.docsReactDevTools)}</p>
<p>${i18nString(UIStrings.docsReactDevToolsDetail)}</p>
</div>
</button>
<button class="rn-welcome-docsfeed-item" type="button" role="link" @click=${this._handleLinkPress.bind(this, 'https:\/\/reactnative.dev/docs/debugging/debugging-native-code')} title="${i18nString(UIStrings.docsNativeDebugging)}">
<button class="rn-welcome-docsfeed-item" type="button" role="link" @click=${this.#handleLinkPress.bind(this, 'https:\/\/reactnative.dev/docs/debugging/debugging-native-code')} title=${i18nString(UIStrings.docsNativeDebugging)}>
<div class="rn-welcome-image" style="background-image: url('${docsImage3Url}')"></div>
<div>
<p class="devtools-link">${i18nString(UIStrings.docsNativeDebugging)}</p>
Expand Down
1 change: 1 addition & 0 deletions scripts/eslint_rules/lib/check_license_header.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ const EXCLUDED_FILES = [
const META_CODE_PATHS = [
'core/host/RNPerfMetrics.ts',
'core/rn_experiments',
'core/sdk/ReactNativeApplicationModel.ts',
'entrypoints/rn_fusebox',
'entrypoints/rn_inspector',
'entrypoints/shell/browser_compatibility_guard.ts',
Expand Down
Loading