Skip to content
This repository has been archived by the owner on Jun 19, 2018. It is now read-only.

Add <ClientSideOnly /> component #20

Closed
wants to merge 1 commit into from
Closed

Add <ClientSideOnly /> component #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 23 additions & 0 deletions src/ClientSideOnly/ClientSideOnly.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { createElement, Component } from 'react';
import { func, node } from 'prop-types';

export default class ClientSideOnly extends Component {
static propTypes = {
onServerRender: func,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

onServerRender is optional for the cases where you really just don't want anything rendered

children: node
};

static defaultProps = {
onServerRender: () => null
};

static isSSR() {
Copy link
Contributor Author

@DrewML DrewML Jan 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is a static method, rather than just an inline check in render, so that a 3rd party SSR implementor could override this logic if they want, and have it automagically applied to every usage of ClientSideOnly in a theme.

Other alternatives include:

  1. The export from this module is a factory fn that takes 1 argument (isSSR function), and returns a ClientSideOnly component

  2. Make people pass isSSR as a prop every time

I chose the static method option because it only requires overriding in 1 place, and I can't think of a good argument for a theme or app having multiple different implementations of isSSR logic

Copy link
Contributor

Choose a reason for hiding this comment

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

The static method works fine. If we need to change it at build time, it's a small change, and it allows for a lot of runtime flexibility.

Of course, prototypal inheritance allows us to make it an instance method and still affect all instances at once...

ClientSideOnly.prototype.isSSR = () => true;

But I think that confuses most people, so your way is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, can be in the proto chain. I tend to only put things on the prototype that rely on instance-specific context, and other related things end up as static properties. Of course, that's purely a (personal) stylistic thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do the same thing. Leave it static!

return process.env.NODE_ENV === 'server';
}

render() {
return ClientSideOnly.isSSR()
? this.props.onServerRender()
: this.props.children;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not the most voracious consumer of React components, so this is a stylistic issue that most people might not have. Paging @jimbo for a second opinion. But I think it might be confusing to take a function-as-prop for server content, but regular old children for client content. If this was an intentional choice to encourage writing client-only content rather than server-only content, I dig it...but I'm still not sure how readable it is.

What if maybe:

import OnlyRender from '..';

// [...]

test('Returns value from invoking serverSide prop in SSR mode', () => {
    jest.spyOn(OnlyRender, 'isSSR').mockImplementation(() => true);
    const wrapper = shallow(
        <OnlyRender
          serverSide={() => <div>Server Content</div>}
          clientSide={() => <span>Client Content</span>}
        />
    );
    expect(wrapper.text()).toEqual('Server Content');
    OnlyRender.isSSR.mockReset();
});

test('Returns default value (null) when serverSide prop is not provided', () => {
    jest.spyOn(OnlyRender, 'isSSR').mockImplementation(() => true);
    const wrapper = shallow(<OnlyRender clientSide={() => <div>Client Content</div>} />);
    expect(wrapper.text()).toEqual('');
    OnlyRender.isSSR.mockReset();
});

test('Returns value from invoking clientSide prop in non-SSR mode', () => {
    jest.spyOn(OnlyRender, 'isSSR').mockImplementation(() => false);
    const wrapper = shallow(
        <OnlyRender
          serverSide={() => <div>Server Content</div>}
          clientSide={() => <span>Client Content</span>}
        />
    );
    expect(wrapper.text()).toEqual('Client Content');
    OnlyRender.isSSR.mockReset();
});

Or what about just letting people specify server-only and client-only content independently?

import OnlyRender from '..';

// [...]

test('Renders children of ServerSide component only in SSR mode', () => {
    jest.spyOn(OnlyRender, 'isSSR').mockImplementation(() => true);
    // OnlyRender itself is a pass-thru for a convenient top-level node
    const wrapper = shallow(
        <OnlyRender>
          <OnlyRender.ServerSide><div>Server Content</div></OnlyRender.ServerSide>
          <OnlyRender.ClientSide><div>Client Content</div></OnlyRender.ClientSide>
        </OnlyRender>
    );
    expect(wrapper.text()).toEqual('Server Content');
    OnlyRender.isSSR.mockReset();
});

test('Renders children of ClientSide component only in non-SSR mode', () => {
    jest.spyOn(OnlyRender, 'isSSR').mockImplementation(() => false);
    const wrapper = shallow(
        <OnlyRender>
          <OnlyRender.ServerSide><div>Server Content</div></OnlyRender.ServerSide>
          <OnlyRender.ClientSide><div>Client Content</div></OnlyRender.ClientSide>
        </OnlyRender>
    );
    expect(wrapper.text()).toEqual('Client Content');
    OnlyRender.isSSR.mockReset();
});

Copy link
Contributor

Choose a reason for hiding this comment

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

(I don't feel especially strongly about this, so if neither of those suggestions are compelling, feel free to brush them off--your API is just fine, and I'm only trying to think about the newbie.)

Copy link
Contributor Author

@DrewML DrewML Jan 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

tbh, the current API is the way it is just because it was the first idea that came to mind.

I'm not a huge fan of the OnlyRender.ClientSide/OnlyRender.ServerSide API, but I have no objections if it's preferred that we use render props for both instead of render prop/children.

I'll go with whichever one of the ideas @jimbo ends up preferring

Copy link
Contributor

Choose a reason for hiding this comment

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

Good discussion. I think @DrewML has the right idea with the original implementation here. This is a pass-through component (render signature of props => props.children) with a single exception (isSSR: true). Idiomatic and easy to follow.

Copy link
Contributor

Choose a reason for hiding this comment

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

10-4, sounds like your current API will be great.

}
}
35 changes: 35 additions & 0 deletions src/ClientSideOnly/__tests__/ClientSideOnly.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import ClientSideOnly from '..';
import { createElement } from 'react';
import { configure, shallow } from 'enzyme';
import Adapter from 'enzyme-adapter-react-16';

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

test('Returns value from invoking onServerRender prop in SSR mode', () => {
jest.spyOn(ClientSideOnly, 'isSSR').mockImplementation(() => true);
const wrapper = shallow(
<ClientSideOnly onServerRender={() => <div>Server Content</div>}>
Client Content
</ClientSideOnly>
);
expect(wrapper.text()).toEqual('Server Content');
ClientSideOnly.isSSR.mockReset();
});

test('Returns default value (null) when onServerRender is not provided', () => {
jest.spyOn(ClientSideOnly, 'isSSR').mockImplementation(() => true);
const wrapper = shallow(<ClientSideOnly>Client Content</ClientSideOnly>);
expect(wrapper.text()).toEqual('');
ClientSideOnly.isSSR.mockReset();
});

test('Returns children in non-SSR mode', () => {
jest.spyOn(ClientSideOnly, 'isSSR').mockImplementation(() => false);
const wrapper = shallow(
<ClientSideOnly onServerRender={() => <div>Server Content</div>}>
Client Content
</ClientSideOnly>
);
expect(wrapper.text()).toEqual('Client Content');
ClientSideOnly.isSSR.mockReset();
});
1 change: 1 addition & 0 deletions src/ClientSideOnly/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from './ClientSideOnly';