2
\$\begingroup\$

I have this class component from the rc-year-calendar library.

import React from "react";
import PropTypes from 'prop-types';
import JsCalendar from "js-year-calendar";
import 'js-year-calendar/dist/js-year-calendar.css';

export default class Calendar extends React.Component {
    static propsTypes = {
        // opsions
        allowOverlap: PropTypes.bool,
        alwaysHalfDay: PropTypes.bool,
        contextMenuItems: PropTypes.arrayOf(PropTypes.shape({
            text: PropTypes.string,
            click: PropTypes.func,
            visible: PropTypes.oneOfType([PropTypes.bool, PropTypes.func]),
            items: PropTypes.array
        })),
        customDayRenderer: PropTypes.func,
        customDataSourceRenderer: PropTypes.func,
        dataSource: PropTypes.oneOfType([
            PropTypes.arrayOf(PropTypes.shape({
                startDate: PropTypes.instanceOf(Date),
                endDate: PropTypes.instanceOf(Date),
                name: PropTypes.string
            })), 
            PropTypes.func
        ]),
        disabledDays: PropTypes.arrayOf(PropTypes.instanceOf(Date)),
        disabledWeekDays: PropTypes.arrayOf(PropTypes.number),
        displayDisabledDataSource: PropTypes.bool,
        displayHeader: PropTypes.bool,
        displayWeekNumber: PropTypes.bool,
        enableContextMenu: PropTypes.bool,
        enableRangeSelection: PropTypes.bool,
        hiddenWeekDays: PropTypes.arrayOf(PropTypes.number),
        language: PropTypes.string,
        loadingTemplate: PropTypes.string,
        maxDate: PropTypes.instanceOf(Date),
        minDate: PropTypes.instanceOf(Date),
        roundRangeLimits: PropTypes.bool,
        selectRange: PropTypes.bool,
        style: PropTypes.string,
        weekStart: PropTypes.number,
        year: PropTypes.number,

        // Events
        onDayClick: PropTypes.func,
        onDayContextMenu: PropTypes.func,
        onDayEnter: PropTypes.func,
        onDayLeave: PropTypes.func,
        onRenderEnd: PropTypes.func,
        onSelectRange: PropTypes.func,
        onYearChanged: PropTypes.func
    };

    static locales = JsCalendar.locales; // Map the "locales" property to the js-year-calendar "locales" property, in order to make the locale files compatible

    constructor(props) {
        super(props);
    }

    componentDidMount() {
        this.JsCalendar = new JsCalendar(this.container, {
            // opsions
            allowOverlap: this.props.allowOverlap,
            alwaysHalfDay: this.props.alwaysHalfDay,
            contextMenuItems: this.props.contextMenuItems,
            customDayRenderer: this.props.customDayRenderer,
            customDataSourceRenderer: this.props.customDataSourceRenderer,
            dataSource: this.props.dataSource,
            disabledDays: this.props.disabledDays,
            disabledWeekDays: this.props.disabledWeekDays,
            displayDisabledDataSource: this.props.displayDisabledDataSource,
            displayHeader: this.props.displayHeader,
            displayWeekNumber: this.props.displayWeekNumber,
            enableContextMenu: this.props.enableContextMenu,
            enableRangeSelection: this.props.enableRangeSelection,
            hiddenWeekDays: this.props.hiddenWeekDays,
            language: this.props.language,
            loadingTemplate: this.props.loadingTemplate,
            maxDate: this.props.maxDate,
            minDate: this.props.minDate,
            roundRangeLimits: this.props.roundRangeLimits,
            style: this.props.style,
            weekStart: this.props.weekStart,
            numberMonthsDisplayed: this.props.numberMonthsDisplayed,
            startYear: this.props.year != null ? this.props.year : this.props.defaultYear,
            startDate: this.props.startDate != null ? this.props.startDate : this.props.defaultStartDate,

            // Events
            clickDay: this.props.onDayClick,
            dayContextMenu: this.props.onDayContextMenu,
            mouseOnDay: this.props.onDayEnter,
            mouseOutDay: this.props.onDayLeave,
            renderEnd: this.props.onRenderEnd,
            selectRange: this.props.onRangeSelected,
            periodChanged: this.props.onPeriodChanged,
            yearChanged: this.props.onYearChanged
        });
    }

    compare(a, b) {
        if (typeof a === "function" && typeof b === "function") {
            return a.toString() != b.toString();
        }
        else if (a instanceof Date && b instanceof Date) {
            return a.getTime() != b.getTime();
        }
        else if (a !== null && typeof a === "object" && b !== null && typeof b === "object") {
            var aKeys = Object.keys(a);
            var bKeys = Object.keys(b);
            
            if (aKeys.length !== bKeys.length) {
                return true;
            }
            else {
                return aKeys.some(key => this.compare(a[key], b[key]));
            }
        }
        else {
            return a != b;
        }
    }

    updateEvent(eventName, oldListener, newListener) {
        this.container.removeEventListener(eventName, oldListener);
        this.container.addEventListener(eventName, newListener);
    }

    componentWillReceiveProps(nextProps) {
        const cal = this.JsCalendar;
        const ops = [];

        // opsions
        if (this.compare(nextProps.allowOverlap, this.props.allowOverlap)) ops.push(() => cal.setAllowOverlap(nextProps.allowOverlap));
        if (this.compare(nextProps.alwaysHalfDay, this.props.alwaysHalfDay)) ops.push(() => cal.setAlwaysHalfDay(nextProps.alwaysHalfDay, true));
        if (this.compare(nextProps.contextMenuItems, this.props.contextMenuItems)) ops.push(() => cal.setContextMenuItems(nextProps.contextMenuItems, true));
        if (this.compare(nextProps.customDayRenderer, this.props.customDayRenderer)) ops.push(() => cal.setCustomDayRenderer(nextProps.customDayRenderer, true));
        if (this.compare(nextProps.customDataSourceRenderer, this.props.customDataSourceRenderer)) ops.push(() => cal.setCustomDataSourceRenderer(nextProps.customDataSourceRenderer, true));
        if (this.compare(nextProps.dataSource, this.props.dataSource)) ops.push(() => cal.setDataSource(nextProps.dataSource, true));
        if (this.compare(nextProps.disabledDays, this.props.disabledDays)) ops.push(() => cal.setDisabledDays(nextProps.disabledDays, true));
        if (this.compare(nextProps.disabledWeekDays, this.props.disabledWeekDays)) ops.push(() => cal.setDisabledWeekDays(nextProps.disabledWeekDays, true));
        if (this.compare(nextProps.displayDisabledDataSource, this.props.displayDisabledDataSource)) ops.push(() => cal.setDisplayDisabledDataSource(nextProps.displayDisabledDataSource, true));
        if (this.compare(nextProps.displayHeader, this.props.displayHeader)) ops.push(() => cal.setDisplayHeader(nextProps.displayHeader, true));
        if (this.compare(nextProps.displayWeekNumber, this.props.displayWeekNumber)) ops.push(() => cal.setDisplayWeekNumber(nextProps.displayWeekNumber, true));
        if (this.compare(nextProps.enableContextMenu, this.props.enableContextMenu)) ops.push(() => cal.setEnableContextMenu(nextProps.enableContextMenu, true));
        if (this.compare(nextProps.enableRangeSelection, this.props.enableRangeSelection)) ops.push(() => cal.setEnableRangeSelection(nextProps.enableRangeSelection, true));
        if (this.compare(nextProps.hiddenWeekDays, this.props.hiddenWeekDays)) ops.push(() => cal.setHiddenWeekDays(nextProps.hiddenWeekDays, true));
        if (this.compare(nextProps.language, this.props.language)) ops.push(() => cal.setLanguage(nextProps.language, true));
        if (this.compare(nextProps.loadingTemplate, this.props.loadingTemplate)) ops.push(() => cal.setLoadingTemplate(nextProps.loadingTemplate, true));
        if (this.compare(nextProps.maxDate, this.props.maxDate)) ops.push(() => cal.setMaxDate(nextProps.maxDate, true));
        if (this.compare(nextProps.minDate, this.props.minDate)) ops.push(() => cal.setMinDate(nextProps.minDate, true));
        if (this.compare(nextProps.roundRangeLimits, this.props.roundRangeLimits)) ops.push(() => cal.setRoundRangeLimits(nextProps.roundRangeLimits, true));
        if (this.compare(nextProps.style, this.props.style)) ops.push(() => cal.setStyle(nextProps.style, true));
        if (this.compare(nextProps.weekStart, this.props.weekStart)) ops.push(() => cal.setWeekStart(nextProps.weekStart, true));
        if (this.compare(nextProps.numberMonthsDisplayed, this.props.numberMonthsDisplayed)) ops.push(() => cal.setWeekStart(nextProps.numberMonthsDisplayed, true));
        if (this.compare(nextProps.startDate, this.props.startDate)) ops.push(() => cal.setStartDate(nextProps.startDate));
        if (this.compare(nextProps.year, this.props.year)) ops.push(() => cal.setYear(nextProps.year));

        // Events
        if (this.compare(nextProps.onDayClick, this.props.onDayClick)) this.updateEvent('clickDay', this.props.onDayClick, nextProps.onDayClick);
        if (this.compare(nextProps.onDayContextMenu, this.props.onDayContextMenu)) this.updateEvent('dayContextMenu', this.props.onDayContextMenu, nextProps.onDayContextMenu);
        if (this.compare(nextProps.onDayEnter, this.props.onDayEnter)) this.updateEvent('mouseOnDay', this.props.onDayEnter, nextProps.onDayEnter);
        if (this.compare(nextProps.onDayLeave, this.props.onDayLeave)) this.updateEvent('mouseOutDay', this.props.onDayLeave, nextProps.onDayLeave);
        if (this.compare(nextProps.onRenderEnd, this.props.onRenderEnd)) this.updateEvent('renderEnd', this.props.onRenderEnd, nextProps.onRenderEnd);
        if (this.compare(nextProps.onRangeSelected, this.props.onRangeSelected)) this.updateEvent('selectRange', this.props.onRangeSelected, nextProps.onRangeSelected);
        if (this.compare(nextProps.onPeriodChanged, this.props.onPeriodChanged)) this.updateEvent('periodChanged', this.props.onPeriodChanged, nextProps.onPeriodChanged);
        if (this.compare(nextProps.onYearChanged, this.props.onYearChanged)) this.updateEvent('yearChanged', this.props.onYearChanged, nextProps.onYearChanged);

        if (ops.length > 0) {
            ops.forEach(op => op());

            if (nextProps.year == this.props.year && nextProps.startDate == this.props.startDate) {
                // If the year has changed, the calendar has automatically been rendered
                cal.render();
            }
        }
    }

    render() {
        return (
            <div ref={elt => this.container = elt}></div>
        );
    }
}

This component is the official React.js wrapper for the js-year-calendar widget.

But the rc-year-calendar library has some peer dependencies issues and I am unable to install and I am using TypeScript, rc-year-calendar has no @types/rc-year-calendar.

So I decided to create my own wrapper since js-year-calendar is written in TypeScript and I made it a function component.

import { useEffect, useRef } from "react";
import Calendar from "js-year-calendar";
import CalendarOptions from "js-year-calendar/dist/interfaces/CalendarOptions";
import CalendarDataSourceElement from "js-year-calendar/dist/interfaces/CalendarDataSourceElement";
import 'js-year-calendar/dist/js-year-calendar.css'

export interface CalendarEvent extends CalendarDataSourceElement {
    details?: string
}

export default function YearCalendar(props?: CalendarOptions<CalendarEvent>) {
    const calendarRef = useRef<any>()
    const previousProps = useRef<CalendarOptions<CalendarEvent> | undefined>()

    useEffect(() => {
        // function properties are not to be compared
        const getDifference = (a: any, b: any) => Object.fromEntries(Object.entries(b).filter(([key, val]) => key in a && a[key] !== val && typeof a[key] !== 'function'));

        if (previousProps.current !== undefined) {
            if (Object.keys(getDifference(previousProps.current, props)).length === 0) {
                return
            }
        }

        previousProps.current = props
        new Calendar(calendarRef.current, props)
    }, [props])

    return <div ref={elt => calendarRef.current = elt} />
}

I want to get some feedback on how well my function component is written and how I can make it better

\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

Looking at your wrapper component, it already seems pretty neat. There are a few things I would change in order to improve readability and maintainability and also add a few changes in terms of typings.

Generally, I'd discourage the usage of any, as it is the root of all evil.

So for example the usage of useRef<any>, would be come something like useRef<HTMLElement | null>.

As far as I've seen it correctly, the Calendar constructor requires string | HTMLElement, which is a bummer. Based on the constructor I'd recommend to go with this quirk and cast it as any, that means we are swallowing the bitter pill there and only there.

For others to keep track of this explicit decision, you could add a comment explaining WHY this decision was made and if needed disable any linters complaining about the any usage.

For example this would like this:

export default function YearCalendar(props?: CalendarOptions<CalendarEvent>) {
    const calendarRef = useRef<HTMLElement | null>()
    // abbreviated for readability
    useEffect(() => {
        /* abbreviated for readability */

        // constructor requires ref to be of type HTMLElement | string rather than HTMLElement | null
        // casting is intended to any is intended to keep the typing throughout this component "correct"
        new Calendar(calendarRef.current as any, props)
    }, [props])

    return <div ref={elt => calendarRef.current = elt} />
}

Which has the advantage, that if there was ever any change or further method accessing and manipulating the ref, it would be properly typed.

The getDifference method is standalone and should be pulled out of the component, this increases readability and it is not allocated everytime the useEffect hook executes.

Avoid nesting multiple ifs together, this is rather ugly and has a high mental complexity. Also, try to express the statements in some more readable manner. Both statements could be summarized to:

when previousProps have a value assigned and previousProps and currentProps are equal then quit the useEffect hook

Applying the above would result in something along those lines:

const getDifference = (a: any, b: any) => Object.fromEntries(Object.entries(b).filter(([key, val]) => key in a && a[key] !== val && typeof a[key] !== 'function'));
const areEqual = (a: any, b: any) => Object.keys(getDifference(a, b)).length === 0;
const isAssigned = <T extends object>(x: T | null | undefined): x is T => x !== undefined && x !== null;

export default function YearCalendar(props?: CalendarOptions<CalendarEvent>) {
    const calendarRef = useRef<HTMLElement | null>()
    const previousProps = useRef<CalendarOptions<CalendarEvent> | undefined>()

    useEffect(() => {
        if (isAssigned(previousProps.current) && areEqual(previousProps.current, props)) {
            return;
        }

        previousProps.current = props
        new Calendar(calendarRef.current as any, props)
    }, [props])

    return <div ref={elt => calendarRef.current = elt} />
}

So combining everything together, this is the code I'd come up with (omitting all comments for abbreviation):

import { useEffect, useRef } from "react";
import Calendar from "js-year-calendar";
import CalendarOptions from "js-year-calendar/dist/interfaces/CalendarOptions";
import CalendarDataSourceElement from "js-year-calendar/dist/interfaces/CalendarDataSourceElement";
import 'js-year-calendar/dist/js-year-calendar.css'

export interface CalendarEvent extends CalendarDataSourceElement {
    details?: string
}

const getDifference = (a: any, b: any) => Object.fromEntries(Object.entries(b).filter(([key, val]) => key in a && a[key] !== val && typeof a[key] !== 'function'));
const areEqual = (a: any, b: any) => Object.keys(getDifference(a, b)).length === 0;
const isAssigned = <T extends object>(x: T | null | undefined): x is T => x !== undefined && x !== null;

export default function YearCalendar(props?: CalendarOptions<CalendarEvent>) {
    const calendarRef = useRef<HTMLElement | null>()
    const previousProps = useRef<CalendarOptions<CalendarEvent> | undefined>()

    useEffect(() => {
        if (isAssigned(previousProps.current) && areEqual(previousProps.current, props)) {
            return;
        }

        previousProps.current = props
        new Calendar(calendarRef.current as any, props)
    }, [props])

    return <div ref={elt => calendarRef.current = elt} />
}

PS: If you notice, that throughout your code base, you want to access previousProps more often in various useEffect hooks, I'd pledge to extract that logic into its own hook calling e.g. usePrevious.

Addendum

As seen in the comments, I didn't clearly express what I meant with the usePrevious hook.

Currently the wrapper manages its previous props on its own. Those two lines are responsible for it:

const previousProps = useRef<CalendarOptions<CalendarEvent> | undefined>();
// somewhere below
previousProps.current = props

So for the wrapper alone, this is a valid solution. As soon as this logic is used more than once (maybe another wrapper component), it might be useful to keep in mind, that it can be extracted.

I personally tend to extract such general functionality in order to keep my stuff clean and follow the single-responsibility pattern.

So for example another component relying on previous props, without extracting it, would then look like this:

export const SomeOtherComponent = (props) => {
  const previousProps = useRef<SomeType>();

  useEffect(() => {
    // DO SOMETHING
    previousProps.current = props
  }, [previousProps]);
}

Extracting this sort of logic, then enables us to concisely describe that we want to access the previousProps of the component without all the technical verbosity and implementation details - as seen above.

Compare the usage below with the usage above:

// somewhere-else.tsx
export const SomeOtherComponent = (props) => {
  const previousProps = usePrevious(props);

  useEffect(() => {
    // DO SOMETHING
  }, [previousProps]);
}

The usage would be similar, in your wrapper component - take a look at the usage without usePrevious and with. Notice that a bunch of useRef related access is now gone:

// YearCalendar.tsx
export default function YearCalendar(props?: CalendarOptions<CalendarEvent>) {
    const calendarRef = useRef<HTMLElement | null>()
    const previousProps = usePrevious(props);

    useEffect(() => {
        if (isAssigned(previousProps) && areEqual(previousProps, props)) {
            return;
        }
        new Calendar(calendarRef.current as any, props)
    }, [props])

    return <div ref={elt => calendarRef.current = elt} />
}

In terms of implementing this hook, an implementation can be found here: https://usehooks.com/usePrevious/ but using the whole package could be a lot of bloat. So it essentially boils down to this:

export const usePrevious<T> = (value: T): T => {
  const ref = useRef<T>();

  useEffect(() => {
    ref.current = value;
  }, [value]);

  return ref.current;
}
\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the review and suggestions, they're really great. However, I couldn't understand the PS observation, how would I extract that logic into its own hook, if you could give any code example it would be much appreciated. \$\endgroup\$ Commented Feb 27, 2023 at 10:33
  • \$\begingroup\$ Of course, I tried to elaborate a bit more on what I meant with the usePrevious hook. Let me know if anything is unclear \$\endgroup\$ Commented Feb 27, 2023 at 13:05
  • \$\begingroup\$ Perfectly explained, thanks for taking the time and give such a detailed description of the code. Really appreciated it. \$\endgroup\$ Commented Feb 28, 2023 at 12:52

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.