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

[bug] (addon-cssresources) - STORY_RENDERED causes picked CSS to reset #6050

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
304 changes: 304 additions & 0 deletions addons/cssresources/src/css-resource-panel.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,304 @@
import React from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason to code these tests in JS and not TS one? If something is not working do not hesitate to tell me and I will take a look!
If not just rename your file to .ts(or even .tsx in your case) 😉

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 specific reason is that these unit tests do not require Typescript for any reason. Can you give me a reason to write these tests in Typescript...or more specifically, how writing these unit tests in Typescript would improve coverage or offer something that Javascript does not.

I'll take it a step further if you'd like. I can eliminate the JSX portion of the test as well by using:

const shallowNode = (props = defaultProps) =>
  shallow(React.createElement(CssResourcePanel, props, null));
const mountNode = (props = defaultProps) =>
  mount(React.createElement(CssResourcePanel, props, null));

Renaming a file to .ts when there is no Typescript seems silly to me. It makes people believe a file contains things that it doesn't.

Copy link
Member

Choose a reason for hiding this comment

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

By writing rename it to .ts I was meaning migrate it to TS. 🔧

I totally agree with you on writing these unit tests in Typescript would not improve coverage nor offer something that Javascript does not ✅ but TS types checking improve productivity and give better maintainability in time.

What are the benefits for me?

  • You can type your defaultProps object with CssResourcePanelProps and so if someone add, remove or update a prop she will instantly see that something is wrong without trying to run tests

  • You will have types checking for jest and enzyme functions and a better intellisense in your IDE

  • Refactoring using IDE features (rename, move, extract and so on) will be much safer and faster

@kroeder @ndelangen what is your opinion about it?

--

Anyway, it's a great and clean PR 👏

Copy link
Member

Choose a reason for hiding this comment

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

Ow I kept tests in JS since i was having difficulty getting jest to work with typescript at some point, If you gents can get it to work 💯 got for it!

There is no reason to keep them .js

Copy link
Contributor Author

@pgoforth pgoforth Mar 15, 2019

Choose a reason for hiding this comment

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

@gaetanmaisse @ndelangen
I would argue that you should always keep your unit tests in vanilla Javascript for the following (very valid) reason.

  • You can type your defaultProps object with CssResourcePanelProps and so if someone add, remove or update a prop she will instantly see that something is wrong without trying to run tests

This is valid for the codebase (which is typed) but not for unit tests. When writing unit tests, you need to be able to have the ability to provide invalid values for anything you pass into your code. This is because you have no control over whether the end user implementing your code is using Typescript to begin with.

  • You will have types checking for jest and enzyme functions and a better intellisense in your IDE

Jest and Enzyme intellisense is provided by your IDE, not Typescript...and by Jest and Enzyme having tTypescript definitions. Since unit tests are not compiled into the codebase, nobody is going to be importing the unit tests, and therefore would not benefit from intellisense on them.

  • Refactoring using IDE features (rename, move, extract and so on) will be much safer and faster

I'm not understanding this and how it applies to a unit test.

All these seems like reasons to develop a codebase in Typescript, but I see using Typescript on your unit tests as a potential detriment because you would lose the ability to test for incorrect types being provided by an end user, which is essential in some cases.

import { configure, mount, shallow } from 'enzyme';
import Adapter from 'enzyme-adapter-react-16';
import { SyntaxHighlighter } from '@storybook/components';
import { STORY_RENDERED } from '@storybook/core-events';
import { EVENTS, PARAM_KEY } from './constants';
import { CssResourcePanel } from './css-resource-panel.tsx';

configure({
adapter: new Adapter(),
});

const defaultParameters = [
{
id: 'fake-css-id-1',
code: 'fake-css-code-1',
picked: true,
},
{
id: 'fake-css-id-2',
code: 'fake-css-code-2',
picked: false,
},
];
const newFakeParameters = [
{
id: 'new-fake-css-id-1',
code: 'new-fake-css-code-1',
picked: false,
},
{
id: 'new-fake-css-id-2',
code: 'new-fake-css-code-2',
picked: true,
},
];
const defaultProps = {
active: true,
api: {
emit: jest.fn(),
on: jest.fn(),
off: jest.fn(),
getParameters: jest.fn(() => defaultParameters),
},
};

const shallowNode = (props = defaultProps) => shallow(<CssResourcePanel {...props} />);
const mountNode = (props = defaultProps) => mount(<CssResourcePanel {...props} />);

describe('CSSResourcePanel', () => {
describe('constructor', () => {
const node = mountNode();

it('should initialize state', () => {
expect(node).toHaveState({ list: [], currentStoryId: '' });
});

it('should not render anything', () => {
expect(node).toBeEmptyRender();
});
});

describe('componentDidMount', () => {
let spy;

afterEach(() => {
spy.mockClear();
});

it('should execute when component is mounted', () => {
spy = jest.spyOn(CssResourcePanel.prototype, 'componentDidMount');
shallowNode();
expect(spy).toHaveBeenCalled();
});

it('should add STORY_RENDERED listener to the api', () => {
const apiAdd = jest.fn();
const node = shallowNode({
...defaultProps,
api: {
...defaultProps.api,
on: apiAdd,
},
});
const { onStoryChange } = node.instance();
expect(apiAdd).toHaveBeenCalledWith(STORY_RENDERED, onStoryChange);
});
});

describe('componentWillUnmount', () => {
let spy;

afterEach(() => {
if (spy) {
spy.mockClear();
}
});

it('should execute when component is unmounted', () => {
spy = jest.spyOn(CssResourcePanel.prototype, 'componentWillUnmount');
shallowNode().unmount();
expect(spy).toHaveBeenCalled();
});

it('should remove STORY_RENDERED listener from the api', () => {
const apiRemove = jest.fn();
const node = shallowNode({
...defaultProps,
api: {
...defaultProps.api,
off: apiRemove,
},
});
const { onStoryChange } = node.instance();
node.unmount();
expect(apiRemove).toHaveBeenCalledWith(STORY_RENDERED, onStoryChange);
});
});

describe('onStoryChange', () => {
it('should populate list with the default items', () => {
const node = shallowNode();
expect(node.state('list')).toMatchObject([]);
node.instance().onStoryChange('fake-story-id');
expect(node.state('list')).toMatchObject(defaultParameters);
});

it('should pull default items from getParameters', () => {
const apiGetParameters = jest.fn(() => newFakeParameters);
const node = shallowNode({
...defaultProps,
api: {
...defaultProps.api,
getParameters: apiGetParameters,
},
});
expect(node.state('list')).toMatchObject([]);
node.instance().onStoryChange('fake-story-id');
expect(apiGetParameters).toHaveBeenCalledWith('fake-story-id', PARAM_KEY);
expect(node.state('list')).toMatchObject(newFakeParameters);
});

it('should maintain picked attribute for matching ids', () => {
const node = shallowNode();
const invertedItem = {
...defaultParameters[0],
picked: !defaultParameters[0].picked,
};
node.setState({ list: [invertedItem] });
node.instance().onStoryChange('fake-story-id');
expect(node.state('list')).toMatchObject([
{
...defaultParameters[0],
picked: !defaultParameters[0].picked,
},
...defaultParameters.slice(1),
]);
});

it("should not overwrite list when story id hasn't changed", () => {
const fakeList = [];
const apiGetParameters = jest.fn(() => newFakeParameters);
const node = shallowNode({
...defaultProps,
api: {
...defaultProps.api,
getParameters: apiGetParameters,
},
});
node.setState({ list: fakeList, currentStoryId: 'fake-story-id' });
expect(node.state('list')).toEqual(fakeList);
node.instance().onStoryChange('fake-story-id');
expect(node.state('list')).toEqual(fakeList);
});

it('should not overwrite list when default list is undefined', () => {
const fakeList = [];
const apiGetParameters = jest.fn(() => undefined);
const node = shallowNode({
...defaultProps,
api: {
...defaultProps.api,
getParameters: apiGetParameters,
},
});
node.setState({ list: fakeList });
expect(node.state('list')).toEqual(fakeList);
node.instance().onStoryChange('fake-story-id');
expect(node.state('list')).toEqual(fakeList);
});
});

describe('onChange', () => {
let spy;

afterEach(() => {
if (spy) {
spy.mockClear();
}
});

const changedIndex = 1;
const fakeEvent = {
target: {
id: defaultParameters[changedIndex].id,
checked: true,
},
};
const newFakeList = defaultParameters.map((param, index) =>
index === changedIndex
? {
...param,
picked: true,
}
: param
);

it('should update the list with new picked items', () => {
const node = shallowNode();
node.instance().onStoryChange('fake-story-id');
node.instance().onChange(fakeEvent);
expect(node.state('list')).toMatchObject(newFakeList);
});

it('should call emit method with updated list', () => {
spy = jest.spyOn(CssResourcePanel.prototype, 'emit');
const node = shallowNode();
node.instance().onStoryChange('fake-story-id');
node.instance().onChange(fakeEvent);
expect(spy).toHaveBeenCalledWith(newFakeList);
});
});

describe('emit', () => {
it('should call emit from the api with EVENTS.SET', () => {
const apiEmit = jest.fn();
const node = shallowNode({
...defaultProps,
api: {
...defaultProps.api,
emit: apiEmit,
},
});
node.instance().emit(newFakeParameters);
expect(apiEmit).toHaveBeenCalledWith(EVENTS.SET, newFakeParameters);
});
});

describe('render', () => {
it('should not render anything when not active', () => {
const node = shallowNode({
...defaultProps,
active: false,
});
node.instance().onStoryChange('fake-story-id');
expect(node).toBeEmptyRender();
});

describe('each list item', () => {
const node = shallowNode();
node.instance().onStoryChange('fake-story-id');

defaultParameters.forEach(param => {
it(`should render list item with id '${param.id}'`, () => {
expect(node.find(`#${param.id}`).length).toEqual(1);
});

it(`should render list item with id '${param.id}' as ${
param.picked ? 'checked' : 'unchecked'
}`, () => {
expect(
node
.find(`#${param.id}`)
.first()
.prop('checked')
).toBe(param.picked);
});
});
});

it('should not render code for items without a code', () => {
const apiGetParameters = jest.fn(() => [
{
id: 'local-fake-id-1',
picked: true,
},
{
id: 'local-fake-id-2',
code: 'local-fake-code-2',
picked: false,
},
]);
const node = shallowNode({
...defaultProps,
api: {
...defaultProps.api,
getParameters: apiGetParameters,
},
});
node.instance().onStoryChange('fake-story-id');
expect(node.find(SyntaxHighlighter).length).toEqual(1);
});
});
});
28 changes: 24 additions & 4 deletions addons/cssresources/src/css-resource-panel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,20 @@ interface CssResourcePanelProps {
}

interface CssResourcePanelState {
currentStoryId: string;
list: CssResource[];
}

interface CssResourceLookup {
[key: string]: CssResource;
}

export class CssResourcePanel extends Component<CssResourcePanelProps, CssResourcePanelState> {
constructor(props: CssResourcePanelProps) {
super(props);

this.state = {
currentStoryId: '',
list: [],
};
}
Expand All @@ -41,12 +47,26 @@ export class CssResourcePanel extends Component<CssResourcePanelProps, CssResour
}

onStoryChange = (id: string) => {
const { list: currentList, currentStoryId } = this.state;
const { api } = this.props;
const list = api.getParameters(id, PARAM_KEY) as CssResource[];

if (list) {
const picked = list.filter(res => res.picked);
this.setState({ list }, () => this.emit(picked));
if (list && currentStoryId !== id) {
const existingIds = currentList.reduce((lookup: CssResourceLookup, res) => {
lookup[res.id] = res;
return lookup;
}, {}) as CssResourceLookup;
const mergedList = list.map(res => {
const existingItem = existingIds[res.id];
return existingItem
? {
...res,
picked: existingItem.picked,
}
: res;
});
const picked = mergedList.filter(res => res.picked);
this.setState({ list: mergedList, currentStoryId: id }, () => this.emit(picked));
}
};

Expand All @@ -65,7 +85,7 @@ export class CssResourcePanel extends Component<CssResourcePanelProps, CssResour
}

render() {
const { list = [] } = this.state;
const { list } = this.state;
const { active } = this.props;

if (!active) {
Expand Down