Consider using createPortal instead of createRoot

[uzegonemad]

In React apps, it is a common practice to have context providers for things like theming components, setting up data fetching, etc.

Currently, slots are rendered using createRoot which creates a completely separate tree without access to an app's providers. This means that when rendering the component in DataTables, it may be unstyled, or may not have access to making API calls, etc.

createPortal solves this problem by rendering the element in a different physical location but retaining it in React's internal tree.

(I started working on a PR for this but couldn't immediately test the slot caching, so decided to toss it in an issue for now. I'll try to come back to it as time allows but if someone else beats me to this, that's fine too.)

A quick example when using Tanstack Query:

function ToggleFlaggedButton({ reportId, entryId, isFlagged }) {
    const { mutateAsync } = useMutation({
        mutationFn: async () => {
            const response = await fetch('/api/toggle-flagged', {
                method: 'POST',
                body: JSON.stringify({ reportId, entryId }),
                headers: { 'Content-Type': 'application/json' },
            });
            if (!response.ok) {
                throw new Error('Something went wrong.');
            }
            return await response.json();
        }
    });

    const onClick = () => {
        const action = mutateAsync({isFlagged: !isFlagged});

        toast.promise(action, {
            loading: 'Toggling flagged...',
            success: `The entry is now ${!isFlagged ? 'flagged' : 'unflagged'}.`,
            error: 'There was an error toggling the flagged status.'
        });
    }

    return (
        <div onClick={onClick} className={'inline-block cursor-pointer'} title={isFlagged ? 'Unflag Entry' : 'Flag Entry'}>{isFlagged ? <FaFlag/> : <FaRegFlag/>}</div>
    )
}

Table slot usage:

const slots = {
    // Flagged
    1: (data, row) => {
        return <ToggleFlaggedButton reportId={reportId} entryId={row.id} isFlagged={data} />
    },
];

<DataTable slots={slots} ...

[AllanJard]

I'm trying to recall why I used createRoot rather than createPortal originally, but failing to do so. Maybe it was an oversight - let's hope so and I can get this included in. I've never liked the createRoot solution. Thanks for brining this up, I'll try to look into it as time allows.

[uzegonemad]

createPortal has to be rendered in a component; I wonder if that's part of the reason the current implementation uses createRoot?

The only way I can currently think of to achieve this would be to add a portals array and make renderJsx push to it, then have <DataTable/>'s render map through and render all the portals.

I cannot imagine that would be performant though. Memoization might help but virtualization/additional logic would probably be needed as well. I haven't confirmed it but I'm guessing this would create portals for every row in a table, not just the ones currently rendered.

[Lexachoc]

Hi, recently I want to render react-router <Link> component in the cell (the goal is to click the link to navigate to different pages), and I got the following error:

TypeError: Cannot destructure property 'basename' of 'React.useContext(...)' as it is null.

This means that the datatables is outside of the react-router context.

Perhaps converting from createRoot to createPortal will solve the problem? Hopefully.

[AllanJard]

@Lexachoc Can you create a test case on Stackbltiz or similar so we can test the fix against?

[Lexachoc]

@AllanJard Sure. I'll create a Stackbltiz example hopefully by Friday.

PS: I thought of one more thing: the Vite hmr. Currently, hmr doesn't work well with the datatable-react. I'm not sure if the conversion can make it work better. Sometimes we have to reload the page (of Stackbltiz) to make the changes to be applied (If this is not related, just ignore me!) It's not that important, just a bit inconvenient.

Example: When changing the rendered component code in the slots, the changes are not applied by hmr.

[uzegonemad]

@Lexachoc I'm using a custom plugin for Vite to work around this. It's really not ideal but it gets the job done. You could simplify it to match any <DataTable if you wanted.

const dtReloader = () => {
    return {
        name: 'dt-reloader',

        // DataTables events don't get correctly updated when HMR updates
        // so this checks if the updated file contained DT events and
        // sends a full-reload event if it's detected
        // We should be able to remove this code if this issue gets fixed: https://github.com/facebook/react/issues/29915
        handleHotUpdate: function({ server, file, modules, read, timestamp }) {
            if(/\.([jt])sx?$/.test(file)) {
                read().then(fileContents => {
                    if(fileContents.includes('current.dt()')) {
                        server.ws.send({ type: 'full-reload' });
                        console.info('[DataTable] Forced full-reload due to detected DataTables event');

                    }
                });
            }

            return modules;
        }
    }
}

[Lexachoc]

@uzegonemad Thanks for the workaround!

[Lexachoc]

Hi @uzegonemad and @AllanJard , please see the Stackbltiz I made.

There is a problem that the slot render doesn't work with createPortal. (I directly use the code from #5.)

@uzegonemad am I doing something wrong?

I will complete this react-router demo as soon as the slot render works as expected.

[uzegonemad]

@Lexachoc Apologies for the delay. I just pushed a change to #5 which should correct this. Could you please try again?

[Lexachoc]

@uzegonemad Hi the new commit works with just one tiny issue.

Please see the new demo here.

In Line 340:

React/src/components/DataTable/index.tsx

Line 340 in eed42c8

const cellData = col.data ? rowData[col.data as string] : null;

This will not work with https://datatables.net/reference/option/columns.title config.
As I am using title instead of data, so the button text ({data}) will always be null.

I replaced this line with:

const cellData = (col.data !== undefined && col.data !== null)
		? rowData[col.data as string]
		: rowData[colIndex];

so it works with my config as a fallback.

P.S. for edge case where col.data could be 0, so I didn't simpify it with const cellData = col.data ? rowData[col.data as string] : rowData[colIndex];. Not sure if I'm being too cautious here, @AllanJard may help confirm.

[Lexachoc]

Hi @uzegonemad , I found there is an problem with row().invalidate() with #5

Please see the demo here.

Previously with createRoot, row().invalidate() would be enough to see the class name changes on the rendered cell.

But now with createPortal, the row().invalidate() is not enough, I have to use row().invalidate().draw(false) to see the cell class name updates. (Technically speaking, I need to trigger a draw event.)

I guess this may due to the use of cache? As a temp fix, I append .draw(false). But ideally it is better to not using draw(false). Not sure if this is possible.

Not sure which is better, I think after using slot function, React should take control over the render, and we simply just row().invalidate() the cell, so the new rendered button can show. Any ideas?

[MikeNeilson]

I tried to use createPortal recently and it didn't seem to work, though I can't exactly remember why - though based on the discussion and the PR I was likely doing it wrong. My usage was in a child row as you can see below so it wasn't the slots render.

By way of work around I created a "Context Wrapper" and custom hook to provide a method to use the contexts to render any component within div in the new root.

https://github.com/opendcs/opendcs/blob/620e3462b03f688d0c3f9dfc3441dffc5e2fc023/javascript/opendcs-web-ui-react/src/pages/sites/SitesTable.tsx#L127

The actual wrapping.
https://github.com/opendcs/opendcs/blob/620e3462b03f688d0c3f9dfc3441dffc5e2fc023/javascript/opendcs-web-ui-react/src/util/ContextWrapper.tsx#L37

Not ideal, seems to always be an extra div, but it does work. I came across this issue looking to see if there was a way to provide that wrapper as a "default", but it would seem that the change in #5 will be better. Sharing in case it helps tide anyone else over until that time.