react icon indicating copy to clipboard operation
react copied to clipboard

Build issue

Open queenvictoria opened this issue 7 months ago • 7 comments

Apologies for this brief and low detail issue. I'll fix it up shortly.

Over the last couple of weeks I've had a mysterious build issue in NextJS. I'll find the error message soon but its about an undefined variable (which should be a string or a function).

Eventually I realised that various committed package-lock.json files worked while others were borked. So then I pinned all the packages in package.json which built perfectly. Iterating through the packages one at a time I discovered that this package was the culprit which I've now pinned to:

    "@phosphor-icons/react": "2.1.7",

I'll test which particular commit causes this issue in versions ^2.1.7 and grab the particular version. Just noting it here for now.

queenvictoria avatar May 26 '25 00:05 queenvictoria

OK. Interested to know if there's an unresolved bug (and we have certainly had bugs in the past) – but FWIW I have no issue with the latest version of Phosphor in a fresh NextJS project:

{
  "name": "nt",
  "version": "0.1.0",
  "private": true,
  "scripts": {
    "dev": "next dev --turbopack",
    "build": "next build",
    "start": "next start",
    "lint": "next lint"
  },
  "dependencies": {
    "@phosphor-icons/react": "^2.1.10",
    "next": "15.3.2",
    "react": "^19.0.0",
    "react-dom": "^19.0.0"
  },
  "devDependencies": {
    "@eslint/eslintrc": "^3",
    "@types/node": "^20",
    "@types/react": "^19",
    "@types/react-dom": "^19",
    "eslint": "^9",
    "eslint-config-next": "15.3.2",
    "typescript": "^5"
  }
}

rektdeckard avatar May 26 '25 03:05 rektdeckard

If you haven't already, I would make sure you are importing from @phosphor-icons/react/ssr in server components (and honestly in general unless you are using IconContext). See: React Server Components and SSR.

rektdeckard avatar May 26 '25 03:05 rektdeckard

Thanks for the recommendation @rektdeckard I will test the ssr import as you suggest. That may address another issue I have had with it. Yes its a weird one. Only happens during next build. And I don't doubt that its a confluence of other things that brings it out.

queenvictoria avatar May 26 '25 19:05 queenvictoria

@rektdeckard there's a bug, it was working before on 2.1.7, now on 2.1.10 it's completely broken for us.

Old code was:

const DynamicIcon = ({ name, ...props }: DynamicIconProps) => {
  if (iconCache.get(name)) {
    const CachedIcon = iconCache.get(name) as React.ComponentType<IconProps>;

    return <CachedIcon {...props} />;
  }

  const Icon = dynamic(() => {
    // code below exists because compile time skyrocket when dev
    // and once saved it needs around 10 seconds or more to update.
    const promise =
      process.env.NODE_ENV === 'production'
        ? import(`@phosphor-icons/react/dist/ssr/${name}`)
        : Promise.resolve(require(`@phosphor-icons/react/dist/ssr`));

    return promise.then((mod) => {
      const icon = mod[name];
      iconCache.set(name, icon);
      return mod[name];
    });
  });

  return <Icon {...props} />;
};

It used to work both on server and client. Now it's broken on server, if I fix it on server it breaks on client.

Gonna investigate further tomorrow.

felipedeboni avatar May 26 '25 22:05 felipedeboni

Well the issue you hinted at in the code comment can probably be solved with optimizePackageImports. In dev server in Turbopack and some other bundlers, imported code is not transpiled and is served as native ESM modules when you import from things like icon libraries. It also can't do any tree-shaking when it does this, meaning you have to load thousands of modules. The config option I pointed you to targets modules for explicit tree-shaking in bot dev and prod.

NextJS has this internally enabled by default for some icon libraries, which is why you may not have noticed it with the most popular icon libraries. We have had a PR open with them for a long time requesting to add our library but nobody seems to want to.

rektdeckard avatar May 26 '25 23:05 rektdeckard

Well the issue you hinted at in the code comment can probably be solved with optimizePackageImports. In dev server in Turbopack and some other bundlers, imported code is not transpiled and is served as native ESM modules when you import from things like icon libraries. It also can't do any tree-shaking when it does this, meaning you have to load thousands of modules. The config option I pointed you to targets modules for explicit tree-shaking in bot dev and prod.

NextJS has this internally enabled by default for some icon libraries, which is why you may not have noticed it with the most popular icon libraries. We have had a PR open with them for a long time requesting to add our library but nobody seems to want to.

I just shared a portion of the code, it was only the component that resolves the icon. The import that is used in production is part of the solution, the other part is done by calling the following function on next.config:

// biome-ignore lint/suspicious/noExplicitAny: too much trouble to type
export const optimizePhosphorIcons = (webpack: any, config: any) => {
  // Add a context replacement plugin for the dynamic imports
  config.plugins.push(
    new webpack.ContextReplacementPlugin(
      /\@phosphor-icons[\/\\]react[\/\\]dist[\/\\]ssr/,
      path.join(process.cwd(), 'node_modules/@phosphor-icons/react/dist/ssr'),
      true,
      /\.\/[A-Za-z0-9_]+$/ // Match imports without extension
    )
  );

  // Set up code splitting for the icons and prevent
  // icons from being included in other chunks
  if (config.optimization?.splitChunks?.cacheGroups) {
    config.optimization.splitChunks.cacheGroups.phosphorIcons = {
      test: /[\\/]node_modules[\\/]@phosphor-icons[\\/]react[\\/]dist[\\/]ssr[\\/][A-Za-z]+\.mjs$/,
      name: (module: { resource?: string }) => {
        const iconName = module.resource?.match(/[\/\\]([A-Za-z]+)\.mjs$/)?.[1];
        return iconName ? `icon-${iconName.toLowerCase()}` : 'phosphor-icon';
      },
      priority: 998,
      chunks: 'async'
    };
  }
};

As for any environment besides production we just require everything because it's way faster HMR.

Here's a screenshot of it running on production, with proper tree-shaking. Worth mentioning it's probably broken now as well given the filename does not ends with .mjs anymore. Image

felipedeboni avatar May 27 '25 03:05 felipedeboni

In that case, you should just be able to update the filenames *.mjs -> *.es.js, no? Also worth noting that the exported symbol names have some changes as of v2.1.8, though filenames (not including the extension) have not.

rektdeckard avatar May 27 '25 03:05 rektdeckard