Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
102 changes: 102 additions & 0 deletions packages/react-devtools-shared/src/__tests__/ComponentSettings-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
import React from 'react';
import { render, unmountComponentAtNode } from 'react-dom';
import ComponentsSettings from '../devtools/views/Settings/ComponentsSettings';

describe('ComponentsSettings', () => {
let container;

beforeEach(() => {
// Set up a DOM element as a render target
container = document.createElement('div');
document.body.appendChild(container);
});

afterEach(() => {
// Clean up on exiting
unmountComponentAtNode(container);
container.remove();
container = null;
});

// @reactVersion >= 17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these annotations, my change has landed on main, so these tests should be included in tests runs now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup i was going to remove it eventually!

it('should update collapseNodesByDefault setting', () => {
const useContextMock = jest.fn().mockReturnValue({
collapseNodesByDefault: true,
addListener: jest.fn(),
removeListener: jest.fn(),
});

render(<ComponentsSettings />, container);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please try to render ComponentsSettings wrapper in required contexts?
Like this:

render(
  <StoreContext.Provider value={...}>
    <SettingsContext.Provider value={...}>
      <ComponentsSettings />
    </SettingsContext.Provider>
  </StoreContext.Provider>,
  container
);

I think this should resolve your problem with mocking contexts and you won't be required to mock useContextMock

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @Biki-das, have you tried this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoxyq let me try :-) , was busy with other stuff!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the issue here is i don't have experience testing react components without rtl 😅, so i am really struggline to make this up!
Plain jest tests are hard to come up with

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no rush, thanks for working on this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoxyq did you looked the test files?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet, sorry, I am finishing something and then will take a look at this, it is still in my queue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried to find a workaround for these tests, but I think we will need to find a better approach on how they should be implemented.

We can remove these tests for now:

  • We don't have a good example with unit tests for custom components, some tests seem outdated and cover integration with user code
  • It is quite painful atm to write tests with using dispatchEvent because of how SyntheticEvents work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup that's what i felt too, when i started out, so for now we would remove the test and get this merged?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup that's what i felt too, when i started out, so for now we would remove the test and get this merged?

Yes, I will review the PR after your changes and hope we can ship this soon :)


const toggleElement = container.querySelector('.Settings input[type="checkbox"]');

expect(toggleElement.checked).toBe(true);

toggleElement.click();

expect(useContextMock().collapseNodesByDefault).toBe(false);
});

it('should update parseHookNames setting', () => {
const setParseHookNamesMock = jest.fn();

const useContextMock = jest.fn().mockReturnValue({
parseHookNames: true,
setParseHookNames: setParseHookNamesMock,
});

render(<ComponentsSettings />, container);

const toggleElement = container.querySelector('.Settings input[type="checkbox"]');

expect(toggleElement.checked).toBe(true);

toggleElement.click();

expect(setParseHookNamesMock).toHaveBeenCalledWith(false);
});

it('should update openInEditorURLPreset setting', () => {
const setLocalStorageMock = jest.spyOn(Storage.prototype, 'setItem');

const useContextMock = jest.fn().mockReturnValue({
openInEditorURLPreset: 'custom',
setOpenInEditorURLPreset: jest.fn(),
});

render(<ComponentsSettings />, container);

const selectElement = container.querySelector('.Settings select');

expect(selectElement.value).toBe('custom');

selectElement.value = 'vscode';
selectElement.dispatchEvent(new Event('change'));

expect(setLocalStorageMock).toHaveBeenCalledWith('OPEN_IN_EDITOR_URL_PRESET', 'vscode');
});

it('should update openInEditorURL setting', () => {
const setLocalStorageMock = jest.spyOn(Storage.prototype, 'setItem');

const useContextMock = jest.fn().mockReturnValue({
openInEditorURLPreset: 'custom',
setOpenInEditorURL: jest.fn(),
});

render(<ComponentsSettings />, container);

const selectElement = container.querySelector('.Settings select');
const inputElement = container.querySelector('.Settings input[type="text"]');

selectElement.value = 'custom';
selectElement.dispatchEvent(new Event('change'));

inputElement.value = 'https://example.com';
inputElement.dispatchEvent(new Event('change'));

expect(setLocalStorageMock).toHaveBeenCalledWith('OPEN_IN_EDITOR_URL', 'https://example.com');
});

// Add additional test cases for other functionality...
});
3 changes: 3 additions & 0 deletions packages/react-devtools-shared/src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ export const SESSION_STORAGE_LAST_SELECTION_KEY =
export const LOCAL_STORAGE_OPEN_IN_EDITOR_URL =
'React::DevTools::openInEditorUrl';

export const LOCAL_STORAGE_OPEN_IN_EDITOR_URL_PRESET =
'React::DevTools::openInEditorUrlPreset';

Comment on lines +42 to +44
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest removing this new config, then if the React::DevTools::openInEditorUrl equals to 'vscode://file/{path}:{line}', you select the VSCode option. That will be simpler.

Copy link
Contributor

@Jack-Works Jack-Works Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @Biki-das will you consider this? also cc @hoxyq

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @Biki-das will you consider this? also cc @hoxyq

I think this adds kinda wrong dependency, you should select the preset first and then based on its value, you either do nothing or update the link template.

For example, if I select "custom" and then put "vscode://file/{path}:{line}", should we automatically switch to VSCode option and hide input?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @Biki-das will you consider this? also cc @hoxyq

I think this adds kinda wrong dependency, you should select the preset first and then based on its value, you either do nothing or update the link template.

For example, if I select "custom" and then put "vscode://file/{path}:{line}", should we automatically switch to VSCode option and hide input?

yup current implementation looks right i think @hoxyq

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, if I select "custom" and then put "vscode://file/{path}:{line}", should we automatically switch to VSCode option and hide input?

Yes. I think this is good, but plz continue this PR if you don't agree with me!

export const LOCAL_STORAGE_PARSE_HOOK_NAMES_KEY =
'React::DevTools::parseHookNames';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,11 @@ export default function InspectedElementWrapper(_: Props): React.Node {
}

const url = new URL(editorURL);
url.href = url.href.replace('{path}', source.fileName);
url.href = url.href.replace('{line}', String(source.lineNumber));
url.href = url.href
.replace('{path}', source.fileName)
.replace('{line}', String(source.lineNumber))
.replace('%7Bpath%7D', source.fileName)
.replace('%7Bline%7D', String(source.lineNumber));
window.open(url);
}, [inspectedElement, editorURL]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ import {
useRef,
useState,
} from 'react';
import {LOCAL_STORAGE_OPEN_IN_EDITOR_URL} from '../../../constants';
import {
LOCAL_STORAGE_OPEN_IN_EDITOR_URL,
LOCAL_STORAGE_OPEN_IN_EDITOR_URL_PRESET,
} from '../../../constants';
import {useLocalStorage, useSubscription} from '../hooks';
import {StoreContext} from '../context';
import Button from '../Button';
Expand Down Expand Up @@ -83,11 +86,17 @@ export default function ComponentsSettings(_: {}): React.Node {
[setParseHookNames],
);

const [openInEditorURLPreset, setOpenInEditorURLPreset] = useLocalStorage<
'vscode' | 'custom',
>(LOCAL_STORAGE_OPEN_IN_EDITOR_URL_PRESET, 'custom');

const [openInEditorURL, setOpenInEditorURL] = useLocalStorage<string>(
LOCAL_STORAGE_OPEN_IN_EDITOR_URL,
getDefaultOpenInEditorURL(),
);

const vscodeFilepath = 'vscode://file/{path}:{line}';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move it outside of function? Same file, but outside of the component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the review! let me do the same :-)


const [componentFilters, setComponentFilters] = useState<
Array<ComponentFilter>,
>(() => [...store.componentFilters]);
Expand Down Expand Up @@ -280,15 +289,32 @@ export default function ComponentsSettings(_: {}): React.Node {

<label className={styles.OpenInURLSetting}>
Open in Editor URL:{' '}
<input
className={styles.Input}
type="text"
placeholder={process.env.EDITOR_URL ?? 'vscode://file/{path}:{line}'}
value={openInEditorURL}
onChange={event => {
setOpenInEditorURL(event.target.value);
}}
/>
<select
className={styles.Select}
value={openInEditorURLPreset}
onChange={({currentTarget}) => {
const selectedValue = currentTarget.value;
setOpenInEditorURLPreset(selectedValue);
if (selectedValue === 'vscode') {
setOpenInEditorURL(vscodeFilepath);
} else if (selectedValue === 'custom') {
setOpenInEditorURL('');
}
}}>
<option value="vscode">VS Code</option>
<option value="custom">Custom</option>
</select>
{openInEditorURLPreset === 'custom' && (
<input
className={styles.Input}
type="text"
placeholder={process.env.EDITOR_URL ? process.env.EDITOR_URL : ''}
Copy link

@subashcs subashcs Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{process.env.EDITOR_URL || ' ' } would be a cleaner approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a try-catch around process.env.EDITOR_URL, it might throw an error because process is undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you possibly help out with the tests, i tried to but could not wrap my head around the structure for the same! The file is already initialized could you try something up :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is an old test for the settings panel and you can add things there

value={openInEditorURL}
onChange={event => {
setOpenInEditorURL(event.target.value);
}}
/>
)}
</label>

<div className={styles.Header}>Hide components where...</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
border: 1px solid var(--color-border);
border-radius: 0.125rem;
padding: 0.125rem;
margin-left: .5rem;
}

.InvalidRegExp,
Expand Down