phosphor-icons / react

A flexible icon family for React
https://phosphoricons.com
MIT License
1.13k stars 60 forks source link

Jest test fails if files are imported from `/dist` folder #62

Open acomanescu opened 1 year ago

acomanescu commented 1 year ago

I have this simple test file:

// File: __tests__/index.test.tsx

import * as React from 'react';
import { render, screen } from '@testing-library/react';

import Page from '../src/page';

import '@testing-library/jest-dom';

describe('Home', () => {
  it('renders a heading', () => {
    render(<Page />);

    const heading = screen.getByRole('heading', {
      name: /Redefining SaaS/i,
    });

    expect(heading).toBeInTheDocument();
  });
});

And this component:

// File: src/page.tsx

import * as React from 'react';
import CheckIcon from '@phosphor-icons/react/dist/icons/Check';

export default function Page() {
  return (
    <div>
      <h1>Redefining SaaS</h1>
      <div>
        <CheckIcon />
      </div>
    </div>
  );
}

Apparently the test fails if I try to import the file from /dist/icons folder.

● Test suite failed to run

    Cannot find module '@phosphor-icons/react/dist/icons/Check' from 'src/page'

It works if I change the import line to:

import { Check as CheckIcon } from '@phosphor-icons/react';

Checking the package.json file of this module, it seems that ./dist/* import is allowed.

rektdeckard commented 1 year ago

Importing from dist is allowed, but only when it is truly an import, and not transpiled to a require, as I suspect is happening in jest. The submodule imports are an undocumented feature and were added to address issues with a few bundlers that do not tree-shake well. Is there a reason you can't keep the destructuring import?

acomanescu commented 1 year ago

@rektdeckard I’m using the package in a Next.js app router project and I need to import them separately due to the tree-shake issue in development.

rektdeckard commented 1 year ago

Just pushed an experimental update that has CJS artifacts and should be natively discoverable by node enviroments. Please give it a try by installing version 2.1.3, or with npm i @phosphor-icons/react@next and LMK if that works.

rektdeckard commented 1 year ago

Note, you'll have to import them from /dist/ssr now

mredigonda commented 1 year ago

Hey @rektdeckard 👋🏼

Joining the thread since I have the exact same use-case for these kind of imports, and the exact same problem as OP (only difference is that I'm not using Next's app router), but sadly changing my imports to be from /dist/ssr instead didn't fix the issue :(

I think probably fixing the type problems mentioned by this other recent thread might help!

It seems no matter how I write the direct imports, it always fails either for the app itself, for the jest tests, or for the Storybook build.

Let me know if I can help in any way, and thanks for looking into this!

rektdeckard commented 1 year ago

@mredigonda it probably fixed the problem but showed another error, that no type declarations were found. That is what I encountered in testing, at least.

Types are now linked correctly thanks to #71 so you should find this to work in 2.0.14.

Note: just use @phosphor-icons/dist/{csr|ssr}/IconName import path

mredigonda commented 1 year ago

@rektdeckard awesome, thank you! Now it seems the code typechecks correctly, storybook and jest all working for me locally, but now I have some error messages when deploying to Vercel:

➤ YN0000: │ pprof@npm:3.2.1 STDERR node-pre-gyp WARN Hit error response status 404 Not Found on https://storage.googleapis.com/cloud-profiler/pprof-nodejs/release/v3.2.1/node-v108-linux-x64-glibc.tar.gz

And

➤ YN0000: │ ssh2@npm:1.11.0 STDOUT Usage Error: Couldn't find a script name "node-gyp" in the top-level (used by ssh2@npm:1.11.0). This typically happens because some package depends on "node-gyp" to build itself, but didn't list it in their dependencies. To fix that, please run "yarn add node-gyp" into your top-level workspace. You also can open an issue on the repository of the specified package to suggest them to use an optional peer dependency.

Are you aware of what the problem could be? Otherwise let me know and I can continue troubleshooting.

Thanks a lot!

rektdeckard commented 11 months ago

@mredigonda this looks like it could be linked to the Next compiler, as I have seen similar errors that trace back to using large component libraries (1000+ files) in their docs. I will try to find them.

Are you deploying on Vercel? Have you tried using modularizeImports or optimizePackageImports options?

mredigonda commented 11 months ago

@rektdeckard thanks for your insight! Yes I'm deploying to Vercel, sadly I don't have any single big package that can be reduced massively by using modularizeImports or optimizePackageImports (the latter is not even available for me as I'm using an older version that doesn't yet have it.

The effort to update the way I import from @phosphor-icons was indeed to improve in this regard, instead of having all icons be accidentally imported, I tried to import each icon separately, but yes at the moment the Vercel deploy just fails 😕.

I'm not sure how to move forward so I've postponed this one, but let me know if you have any ideas on what else I could try, and thanks again for your replies!