ChangeTheme: Sometimes leaves 2 or more links in the `<head>`

bloodykheeng commented 5 months ago

Describe the bug

ive noted that everytime i call change theme it duplicates the link element am refereing to in the head section so which causes me issuses when am changing the theme though i had the manually first always delete exsting nodes by doing this if (existingLinks.length > 1) { // Assuming that the first link is the old one and the new one is appended last, // we remove the first occurrence. document.head.removeChild(existingLinks[0]); } });

here is my whole code

import React, {
  createContext,
  useContext,
  useState,
  useEffect,
  useCallback
} from "react";
import { PrimeReactContext } from "primereact/api"; // Import the context
import PrimeReact from "primereact/api";

const ThemeContext = createContext();

export const ThemeProvider = ({ children }) => {
  const [theme, setTheme] = useState(
    () => localStorage.getItem("theme") || "light"
  );

  //   const [theme, setTheme] = useState("dark");
  const { changeTheme } = useContext(PrimeReactContext); // Access PrimeReact API through context

  const getThemeFromLocalStorage = () => {
    const linkId = "theme-link";
    console.log("current theme is : ", theme);
    changeTheme(`lara-light-blue`, `lara-${theme}-blue`, linkId, () => {
      const existingLinks = document.querySelectorAll(`link[id="${linkId}"]`);
      if (existingLinks.length > 1) {
        // Assuming that the first link is the old one and the new one is appended last,
        // we remove the first occurrence.
        document.head.removeChild(existingLinks[0]);
      }
    });
    return;
  };

  const changeMyTheme = () => {
    const newTheme = theme === "dark" ? "light" : "dark";
    const linkId = "theme-link";
    changeTheme(`lara-${theme}-blue`, `lara-${newTheme}-blue`, linkId, () => {
      setTheme(newTheme);
      localStorage.setItem("theme", newTheme);
      console.log("current theme in local storage : ", newTheme);
    });
    return;
  };

  let myTheme = theme;

  useEffect(() => {
    getThemeFromLocalStorage();
  }, []);

  const toggleTheme = () => {
    let currentTheme;
    if (theme === "light") {
      currentTheme = "dark";
    } else {
      currentTheme = "light";
    }

    // setTheme(currentTheme);
    // setTheme((current) => (current === "light" ? "dark" : "light"));
    changeMyTheme();
  };

  return (
    <ThemeContext.Provider value={{ theme, toggleTheme }}>
      {children}
    </ThemeContext.Provider>
  );
};

export const useTheme = () => useContext(ThemeContext);

here is the link element am refering two in the dom head section

  <link id="theme-link" rel="stylesheet" href="/themes/lara-light-blue/theme.css">

  <title>React App</title>
</head>

i dono if this is a bug or what but i hope this get solved.

Reproducer

No response

PrimeReact version

10.6.2

React version

17.x

Language

ES6

Build / Runtime

Create React App (CRA)

Browser(s)

chrome

Steps to reproduce the behavior

No response

Expected behavior

I expect the change theme to just reference to the link element in the head and it changes the path not creating new link element every time i call it in a different function

sja-cslab commented 5 months ago

I don't really understand what exactly you're doing there. However, I don't think that this is something PrimeReact could fix. Why are you doing things so complicated?

Just do something like:

<link id="theme" rel="stylesheet" type="text/css" href="/my/default/theme.css">

let themeLink = document.getElementById('theme');

if (themeLink) {
    themeLink.href = `/themes/${themeId}/theme.css`;
}

bloodykheeng commented 5 months ago

am changing between dark and light theme following prime react docs your solution was what i tried first but the problem with it during transition or the process of changing the theme you can see the elements unstyled for like 2 seconds before a theme gets updated which makes it wrong approach i think

github-actions[bot] commented 5 months ago

Please fork the Stackblitz project and create a case demonstrating your bug report. This issue will be closed if no activities in 20 days.

melloware commented 5 months ago

@bloodykheeng so you are saying it has 2 <link> in the head and you are manually removing one. Are you saying your code posted above fixes the problem and the docs need updating OR you still have the issue? a reproducer will help.

sja-cslab commented 5 months ago

@melloware i can reproduce it on the doc.

However, it took a while to switch themes over and over and it felt like i was too fast one time which causes this

melloware commented 5 months ago

ahhh it might be a timing thing with the hooks and order its firing...

sja-cslab commented 5 months ago

bloodykheeng may work in strict mode which causes double render then?

Edit: changeTheme as useCallback could do the trick!?

sja-cslab commented 5 months ago

@bloodykheeng you're right - if you dont have any fallback theme, the switch can cause a moment of unstyle thing because the css is loaded over the network

melloware commented 5 months ago

@sja-cslab let me know if we need to fix the code or update the docs or both.

bloodykheeng commented 5 months ago

@melloware @sja-cslab What i noted is that every time i call this changeTheme it creates a link element in the head of the document if u have only one function that calls changeTheme every thing works fine but if u call it in another place that will invoke two links in that head thats why in my code i had to put a call back that manually removes one link from the head const getThemeFromLocalStorage = () => { const linkId = "theme-link"; console.log("current theme is : ", theme); changeTheme(lara-light-blue,lara-${theme}-blue, linkId, () => { const existingLinks = document.querySelectorAll(link[id="${linkId}"]); if (existingLinks.length > 1) { // Assuming that the first link is the old one and the new one is appended last, // we remove the first occurrence. document.head.removeChild(existingLinks[0]); } }); return; };

see the call back in the above function i use that function to get the theme i stored in local storage on initial mount so it gets it and sets it fine but now after that i have this other one too

const changeMyTheme = () => { const newTheme = theme === "dark" ? "light" : "dark"; const linkId = "theme-link"; changeTheme(lara-${theme}-blue,lara-${newTheme}-blue, linkId, () => { setTheme(newTheme); localStorage.setItem("theme", newTheme); console.log("current theme in local storage : ", newTheme); }); return; };

the above changes the theme everytime user clicks on a button in the app so i notted that using two changeTheme hooks in my context creates two links of the same id in the head which causes an accident when the app css styles are running it ends up that this changeMyTheme() will never work correctly because it will be changing the first link so that why i said i had to manually remove a one link from the head section on intial load after applying the theme from users local storage by doing this if (existingLinks.length > 1) { // Assuming that the first link is the old one and the new one is appended last, // we remove the first occurrence. document.head.removeChild(existingLinks[0]); }

here is my entire code i had placed in the context

`import React, { createContext, useContext, useState, useEffect, useCallback } from "react"; import { PrimeReactContext } from "primereact/api"; // Import the context import PrimeReact from "primereact/api";

const ThemeContext = createContext();

export const ThemeProvider = ({ children }) => { const [theme, setTheme] = useState( () => localStorage.getItem("theme") || "light" );

// const [theme, setTheme] = useState("dark"); const { changeTheme } = useContext(PrimeReactContext); // Access PrimeReact API through context

let myTheme = theme;

useEffect(() => { getThemeFromLocalStorage(); }, []);

const toggleTheme = () => { let currentTheme; if (theme === "light") { currentTheme = "dark"; } else { currentTheme = "light"; }

// setTheme(currentTheme);
// setTheme((current) => (current === "light" ? "dark" : "light"));
changeMyTheme();

};

return ( <ThemeContext.Provider value={{ theme, toggleTheme }}> {children} </ThemeContext.Provider> ); };

export const useTheme = () => useContext(ThemeContext); `

sja-cslab commented 5 months ago

@melloware, the docs are just fine - I would say it's a bug.

And yeah, it seems to be a timing problem. The following happens:

1: Find the link element. 2: Clone it. 3: Set the ID of the clone to oldId-clone and set the href to the new URL. 4:Add a "load" event listener (async). 5: Insert the clone before the original link. 6: Loading is done - remove the old element and remove "-clone" from the clone's ID.

The issue arises with steps 5 and 6. If a second clone is created and inserted right before the loaded event triggers, we end up with two clones, both of which are renamed to the original ID.

Question here is, how would you like to prevent that? We could check for an already available clone e.g.

// Check if a cloned link element already exists
    const existingCloneElement = document.getElementById(linkElementId + '-clone');

     // If a cloned link element already exists, remove it from the document
     existingCloneElement?.remove();

bloodykheeng commented 5 months ago

@sja-cslab well i think u should make sure after updating the link path then either delete the original or the clone such that we don't end up with two links of same id pointing to same css styles

sja-cslab commented 5 months ago

@bloodykheeng if you delete the original element before the load, you'll end up with the effect, that you got a timespan without styling

bloodykheeng commented 5 months ago

@sja-cslab see how i did it by deleting the parent in the frrom the changeTheme call back const existingLinks = document.querySelectorAll(link[id="${linkId}"]); if (existingLinks.length > 1) { // Assuming that the first link is the old one and the new one is appended last, // we remove the first occurrence. document.head.removeChild(existingLinks[0]);

then is the whole

const getThemeFromLocalStorage = () => { const linkId = "theme-link"; console.log("current theme is : ", theme); changeTheme(lara-light-blue,lara-${theme}-blue, linkId, () => { const existingLinks = document.querySelectorAll(link[id="${linkId}"]); if (existingLinks.length > 1) { // Assuming that the first link is the old one and the new one is appended last, // we remove the first occurrence. document.head.removeChild(existingLinks[0]); } }); return; };

sja-cslab commented 5 months ago

I see what you're doing there, but you can't be sure that this would always work. As you say in the comment, "Assuming," which should not be used here. Because in different timing/callchain scenarios, this could still cause the wrong theme to load. So it's better to avoid that as soon as possible.

In your example, let's say you load theme A, then B, but B loads faster (because, for example, it's smaller), so you would delete B even if the user swapped to it last

PS: I'm not sure if that example is correct but I think it explains what could happen

bloodykheeng commented 5 months ago

@sja-cslab ur right i think what prime react team members should do is to change the change theme logic in that were as it clones let it alwas delete the first link that is at index 0 if it exists or else lets change this strategy of manipulating the dom and just apply a layer in some config.js like how tailwind does

rahulAnand1999 commented 2 months ago

Hii i am storing the theme in my db,,when i get it from the server i want to change the theme , the colorscheme gets changed but the theme color remains the same...tell me where i am doing wrong?

@nitrogenous can you help?

const { data: currentUserTheme } = useSpecificUserTheme(); const invalidateUserTheme = useInvalidateSpecificUserTheme(); const updateUserThemeMutation = useUpdateUserTheme({ onSuccess: () => invalidateUserTheme() }); const [breadcrumbs, setBreadcrumbs] = useState<Breadcrumb[]>([]); const [layoutConfig, setLayoutConfig] = useState(defaultLayoutConfig); const [layoutState, setLayoutState] = useState(defaultLayoutState); const { setRipple, changeTheme } = useContext(PrimeReactContext);

useEffect(() => {
    if (Object.keys(currentUserTheme || {}).length ) {
        const { colorScheme,theme } = currentUserTheme;
        changeTheme(layoutConfig.colorScheme, `${colorScheme}-${theme}`, 'theme-link', () => {
            setLayoutConfig(currentUserTheme);
        });
    }
}, [currentUserTheme]);

rahulAnand1999 commented 2 months ago

 * This method is used to change the theme dynamically.
 * @param {string} theme - The name of the theme to be applied.
 * @param {string} newTheme - The name of the new theme to be applied.
 * @param {string} linkElementId - The id of the link element to be updated.
 * @param callback - Callback to invoke when the theme change is completed.
 */
changeTheme?(theme?: string, newTheme?: string, linkElementId?: string, callback?: () => void): void;

this documentation is very confusing for theme and new theme

can anyone help asap?

melloware commented 2 months ago

@rahulAnand1999 you change it including the color scheme like..

changeTheme('lara-light-blue', 'bootstrap4-dark-purple', 'theme-link', () => {
            setLayoutConfig(currentUserTheme);
        });

The them URLs look like...

primereact/resources/themes/mdc-dark-deeppurple/theme.css
primereact/resources/themes/tailwind-light/theme.css
primereact/resources/themes/fluent-light/theme.css
primereact/resources/themes/lara-light-blue/theme.css
primereact/resources/themes/lara-light-indigo/theme.css

You are finding and replacing the entire 4th section of that url.

melloware commented 2 months ago

@bloodykheeng @sja-cslab @rahulAnand1999 submitted a PR to improve the docs as well not do an insert and delete link but just replace the link in place. It seems to be working using the showcase.

primefaces / primereact