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.

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(component): no destroy as not all listeners created in React #300

Merged
merged 3 commits into from
Oct 9, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
8 changes: 6 additions & 2 deletions src/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,12 @@ export default function Component(props) {
// Fire the unmount before we destroy the component so that the event is not lost
comp.emitChange('unmount');

// Remove all listeners to prevent listener leaks
comp.destroy();
// We don't want to destroy the component as this could remove listeners that were not
// created in the React layer. For example, a form listens to all its fields and destroying
// the form will cause it to stop listening to its fields. Instead, the React layer should
// only remove listeners that it creates.
//
// comp.destroy();
}
};
}, [comp]);
Expand Down
89 changes: 50 additions & 39 deletions src/fields/collection-field.test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { fireEvent, waitFor } from '@testing-library/react';
import { fireEvent, waitFor, screen } from '@testing-library/react';
import { compileAndRender } from '../test-utils';
import { makeDnd, DND_DIRECTION_DOWN } from 'react-beautiful-dnd-test-utils';

Expand Down Expand Up @@ -37,10 +37,10 @@ const contacts = [
},
];

const expectContactsToEqual = async (getAllByLabelText, contacts) => {
const expectContactsToEqual = async (contacts) => {
await waitFor(
() => {
const nodes = getAllByLabelText(/First Name/);
const nodes = screen.getAllByLabelText(/First Name/);
expect(nodes.map((node) => node.textContent)).toEqual(contacts);
},
{
Expand All @@ -51,97 +51,108 @@ const expectContactsToEqual = async (getAllByLabelText, contacts) => {
};

it('should list', async () => {
const { getAllByLabelText } = compileAndRender(definition, contacts);
compileAndRender(definition, contacts);

await expectContactsToEqual(getAllByLabelText, ['Daenerys', 'Jon', 'Tyrion']);
await expectContactsToEqual(['Daenerys', 'Jon', 'Tyrion']);
});

it('should create', async () => {
const { findByLabelText, getByRole, getAllByLabelText } = compileAndRender(
definition
);
const shouldCreate = async () => {
const renderResult = compileAndRender(definition);

// Click "New Contact" button
const newContact = getByRole('button', { name: /New Contact/i });
const newContact = screen.getByRole('button', { name: /New Contact/i });
fireEvent.click(newContact);

// Fill in First Name
const firstName = await findByLabelText(/First Name/);
const firstName = await screen.findByLabelText(/First Name/);
fireEvent.change(firstName, { target: { value: 'Ray' } });

// Save the form
const save = getByRole('button', { name: /Save/i });
const save = screen.getByRole('button', { name: /Save/i });
fireEvent.click(save);

// Verify that the contact now appears in the list
await expectContactsToEqual(getAllByLabelText, ['Ray']);
await expectContactsToEqual(['Ray']);

return renderResult;
};

it('should create', async () => {
await shouldCreate();
});

const populateList = async (def) => {
const renderResult = compileAndRender(
def === undefined ? definition : def,
contacts
);
const { getAllByLabelText } = renderResult;
await expectContactsToEqual(getAllByLabelText, ['Daenerys', 'Jon', 'Tyrion']);
await expectContactsToEqual(['Daenerys', 'Jon', 'Tyrion']);
return renderResult;
};

it('should view', async () => {
const { getAllByLabelText, getByRole } = await populateList();
await populateList();

// Click to view first item
const view = getByRole('button', { name: /View.*daenerys/i });
const view = screen.getByRole('button', { name: /View.*daenerys/i });
fireEvent.click(view);

// Verify that item is being displayed. The extra "Daenerys" is the contact displayed in the
// FormDialog. TODO: is there a better way to perform this check?
await expectContactsToEqual(getAllByLabelText, [
'Daenerys',
'Jon',
'Tyrion',
'Daenerys',
]);
await expectContactsToEqual(['Daenerys', 'Jon', 'Tyrion', 'Daenerys']);
});

it('should edit', async () => {
const {
getAllByLabelText,
getByRole,
findByLabelText,
} = await populateList();

const edit = async (id, firstName) => {
// Edit item
const edit = getByRole('button', { name: /Edit.*daenerys/i });
const edit = screen.getByRole('button', { name: RegExp(`Edit.*${id}`, 'i') });
fireEvent.click(edit);

// Fill in First Name
const firstName = await findByLabelText(/First Name/, { selector: 'input' });
fireEvent.change(firstName, { target: { value: 'Sansa' } });
const first = await screen.findByLabelText(/First Name/, {
selector: 'input',
});
fireEvent.change(first, { target: { value: firstName } });

// Save the form
const save = getByRole('button', { name: /Save/i });
const save = screen.getByRole('button', { name: /Save/i });
fireEvent.click(save);
};

it('should edit', async () => {
await populateList();

await edit('daenerys', 'Sansa');

// Verify that the contact now appears in the list
await expectContactsToEqual(['Sansa', 'Jon', 'Tyrion']);
});

it('should create and edit first time', async () => {
const { component } = await shouldCreate();

const id = component.getValue()[0].id;

await edit(id, 'Sansa');

// Verify that the contact now appears in the list
await expectContactsToEqual(getAllByLabelText, ['Sansa', 'Jon', 'Tyrion']);
await expectContactsToEqual(['Sansa']);
});

it('should reorder', async () => {
const { getAllByLabelText, getByText, getByLabelText } = await populateList({
await populateList({
...definition,
forbidOrder: false,
});

// Drag daenerys down by one position
await makeDnd({
getByText,
getDragEl: () => getByLabelText(/Drag.*daenerys/i),
getByText: screen.getByText,
getDragEl: () => screen.getByLabelText(/Drag.*daenerys/i),
direction: DND_DIRECTION_DOWN,
positions: 1,
});

await expectContactsToEqual(getAllByLabelText, ['Jon', 'Daenerys', 'Tyrion']);
await expectContactsToEqual(['Jon', 'Daenerys', 'Tyrion']);
});

// TODO: archive
Expand Down
2 changes: 1 addition & 1 deletion src/test-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ export const compileAndRender = (definition, value) => {
component.setValue(value);
}

return render(<Component component={component} />);
return { ...render(<Component component={component} />), component };
};