BUG: Too many open files on Next 13

louisgv commented 1 year ago

Prerequisites

Version: 6.2.0
Are you running from source/main: No
Are you using a released build: Yes
Operating system: Deployed on Vercel

Step to reproduce

Upgrade to 6.2.0
Use tons of Icon everywhere
Deploy to Vercel

Actual behavior

Build passed on Vercel, but server-side rendering/rehydration throws the error below

NOTE: Dev/Build is working just fine on the local machine

Any message or error

2023-02-05T23:21:36.825Z
ERROR   [Error: EMFILE: too many open files, open '/var/task/node_modules/.pnpm/iconoir-react@6.2.0_react@18.2.0/node_modules/iconoir-react/dist/esm/ShareIos.mjs'] {
  errno: -24,
  code: 'EMFILE',
  syscall: 'open',
  path: '/var/task/node_modules/.pnpm/iconoir-react@6.2.0_react@18.2.0/node_modules/iconoir-react/dist/esm/ShareIos.mjs',
  page: '/'
}
RequestId: d7fb0c90-4232-4896-8c8a-0e1598ad436e Error: Runtime exited without providing a reason
Runtime.ExitError

Additional info or screenshots

Downgrading to 6.1.0 fixed the issue. This is likely introduced by https://github.com/iconoir-icons/iconoir/pull/240

Upvote & Fund

We're using Polar.sh so you can upvote and help fund this issue.
We receive the funding once the issue is completed & confirmed by you.
Thank you in advance for helping prioritize & fund our backlog.

louisgv commented 1 year ago

Note: I'm not using the /app directory. My next project uses just the pages directory at the moment.

sammarks commented 1 year ago

I would check out this StackOverflow post on someone experiencing a similar issue with Material UI icons. It offers a different way of importing the icons as a solution.

While we could package all icons in one file to get around this, we have to package each icon individually in order to support tree shaking with Webpack, along with allowing users to import just one icon without importing the entire icon set.

As another way of getting around this, I recommend increasing the nofile setting on your machine. Here's how to increase it on Linux and MacOS. You might find yourself running into this limit again, especially as your project continues to grow.

--

As for how to increase that limit on Vercel itself, I can't find any resources to help with that, so I would recommend just importing each icon individually. It might be worth reaching out to them to see what they recommend when running into this issue, especially since it appears to be happening to MUI icons too.

sammarks commented 1 year ago

These also seem related:

Which is strange because we just overhauled our packaging to be more compatible with ESM.

Happy to accept a PR if you're able to figure out if we're packaging something incorrectly but until then I recommend just importing the icons you need individually.

jagged91 commented 1 year ago

I'm actually seeing the same issue on my server, even with importing files individually, like so:

import { Search } from 'iconoir-react'

This is the correct syntax to avoid importing the entire library, right? Note that I'm also using Mantine and Tabler icons (Mantine rich text editor dependency).

louisgv commented 1 year ago

More context, here're some sample of me importing the icons:

import { KeyAlt } from "iconoir-react"

import { SelectiveTool } from "iconoir-react"

import { CheckCircle, WarningCircle } from "iconoir-react"

louisgv commented 1 year ago

I would check out this StackOverflow post on someone experiencing a similar issue with Material UI icons. It offers a different way of importing the icons as a solution.

While we could package all icons in one file to get around this, we have to package each icon individually in order to support tree shaking with Webpack, along with allowing users to import just one icon without importing the entire icon set.

As another way of getting around this, I recommend increasing the nofile setting on your machine. Here's how to increase it on Linux and MacOS. You might find yourself running into this limit again, especially as your project continues to grow.

--

As for how to increase that limit on Vercel itself, I can't find any resources to help with that, so I would recommend just importing each icon individually. It might be worth reaching out to them to see what they recommend when running into this issue, especially since it appears to be happening to MUI icons too.

Interesting, so I'd be importing it like this:

import CheckCircle from "iconoir-react/dist/CheckCircle"

Which is weird... I'd have thought vercel/webpack to perfected the tree shaking part

sammarks commented 1 year ago

Which is weird... I'd have thought vercel/webpack to perfected the tree shaking part

I would have thought so too 😅 but it appears either we have something setup in a way NextJS doesn't like (I suspect it has to do with how the exports are setup), or there's a bug in the way their tree shaking is working currently.

That import, as well as the following variations will work:

import CheckCircle from 'iconoir-react/dist/CheckCircle'
import CheckCircle from 'iconoir-react/dist/esm/CheckCircle'

// If you are using this icon in a server component, use the /server/ export, which
// strips out the usage of IconoirContext because you can't use React context in
// server components.
import CheckCircle from 'iconoir-react/dist/esm/server/CheckCircle'

louisgv commented 1 year ago

Hmm, so it seems that NextJS traverse ESM import by looking at the exports field. After trying the above in a couple area, I'm getting this error:

Module not found: Package path ./dist/GitHub is not exported from package /vercel/path0/apps/node_modules/iconoir-react (see exports field in /vercel/path0/apps/node_modules/iconoir-react/package.json)

louisgv commented 1 year ago

Hmm, the react package should remove the export declaration for now I think, (or somehow add a glob pathing to all the icon). Apparently, MUI-icon has no exports: https://www.npmjs.com/package/@mui/icons-material?activeTab=explore

https://github.com/mui/material-ui/blob/master/packages/mui-icons-material/package.json

So it's likely that nextjs ESM resolver short circuit the pathing by looking at the exports first.

louisgv commented 1 year ago

Hmm, it seems to me that because of the exports, the whole package is not being tree-shake at all.

louisgv commented 1 year ago

Yup - which is indeed the case. That's the key diff that mattered between 6.1.1 vs 6.2.0 - the exports statement! Downgrading to 6.1.1 works

sammarks commented 1 year ago

The problem is if you remove the exports field, then using Iconoir inside server components in the app directory breaks 😉

Looks like we'll end up having to figure out a glob solution or manually specify each icon in the exports field.

Mitsunee commented 1 year ago

Which is weird... I'd have thought vercel/webpack to perfected the tree shaking part

sadly this is not quite the case yet for Next, which is still CJS under the hood (which you can probably blame jest for). I've had a problem with a different package where I was importing some enums and it import the entire index.js with all enums and an entire API connector that I was not using. So in the case of iconoir it'd be importing every icon which is probably a lot of files. I can recommend @next/bundle-analyzer for looking into this in more detail if you'd like.

The only workaround I've found is using the individual file paths or having a package use the exports field in package.json (I wonder if this could be automated?). Here's an example from an astro project where I've also used the individual import workaround (mostly out of habit tbh):

https://github.com/Mitsunee/mitsu-portfolio/blob/d457962d2e2b1c53da8745716d4df1ee42b18499/src/layouts/PageWithHero.astro#L1-L6

sammarks commented 1 year ago

@Mitsunee Do you have an example of a lib using the exports approach successfully where the tree-shaking is working perfectly inside Next? That would be really helpful in updating our approach!

Mitsunee commented 1 year ago

@sammarks Kind of, but it broke TypeScript for me and I haven't messed with it in a while (I did get an email with a suggestion, but haven't tried it yet). My (hopefully temporary) workaround for my node-util package was to have d.ts files for each export in the root that re-export the generated files in the dist directory (see fs.d.ts for an exaple), which is not really a scalable solution.

The basic idea behind the exports field would be this, but I've had issues getting TypeScript to read d.ts files in directories (I also generate mine into a dist directory actually). Common advice has just been to put the "types" key on top (see example below) and tell users to use a different moduleResolution method in tsconfig (which is usually not acceptable). Not sure if this has improved with TypeScript 4.9.

{
  "types": "./dist/index.d.ts",
  "main": "./dist/index.js",
  "module": "./dist/esm/indes.mjs",
  "exports": {
    "./": {
      "types": "./dist/index.d.ts",
      "require": "./dist/index.js",
      "import": "./dist/esm/index.mjs"
    },
    "./CheckCircle": {
      "types": "./dist/CheckCircle.d.ts",
      "require": "./dist/CheckCircle.js",
      "import": "./dist/esm/CheckCircle.mjs"
    },

The email I've got suggested also adding this to package.json, which apparently helps TypeScript locate the files:

{
  "typesVersions": {
    "*": {
      "*": ["./dist/*"]
    }
  }
}

I have tried this (and thought multiple times that I figured it out, finally, after over a year) but this breaks the "." export because the "*" wildcard always overrides ".". The TypeScript documentation literally lies about it too, claiming the order matters, but no, it doesn't.

sammarks commented 1 year ago

@Mitsunee I noticed in an email you had a comment that had a code snippet like the following:

{
  "exports": {
    // ...
    "./*": {
      "types": "./dist/*.d.ts",
      "import": "./dist/esm/*.mjs",
      "require": "./dist/*.js"
    }
  }
}

Did you end up doing some tests and that didn't work? That would be the best case, if it does indeed work (I should also do some testing to see if I can get it working). Otherwise I'd agree your proposed solution of generating the package.json that (while unsightly) is gonna be the most scalable solution.

Mitsunee commented 1 year ago

@sammarks yes, this approach does work for Node.js, but TypesSript is currently not able to use this "types" property properly. I was trying around with using typesVersions to emulate it, but ended up either breaking the from "iconnoir-react"; import or having false positives resolve to ./dist/index.d.ts such as from "iconnoir-react/ThisDoesntExist"; which I had noticed after having already sent the comment.

Sorry for the mess of emails/pings I probably caused with that, I got a little overexcited in thinking I had finally figured out an issue I've been trying to solve for many months now.

I agree that autogenerating package.json is a bit of an ugly solution as well (assuming it even works), ~~I wonder if it would be possible to autogenerate a d.ts file that contains declare module "iconoir-react/IconNameHere" statements~~ (nevermind that, ambient modules can't re-export from relative paths) instead to still allow for the use of wildcards, but I'm unsure if this would solve the issue of false positives I mentioned above.

I also looked into how other libraries deal with the issue some more and they all seem to either have a manually authored types.d.ts (or similar name) in the project root or just do not use the exports field at all. I really hope the developers of TypeScript eventually just realize that this should just work out of the box for every type of moduleResolution setting, but I have honestly given up trying to communicate the issue.

sammarks commented 1 year ago

And thus we have the current state of NodeJS module resolution 😅 - I appreciate the research on this! When I have a bit of time I'll play around with it but I can't imagine I'll find anything better than you have already.

louisgv commented 1 year ago

I think that with the state of nextjs's custom resolver, TS's resolver, and the nodejs ESM native resolver... It's gonna take 2 more years for a consensus to be reached.

I think the best solution for now is just remove the exports field for the react package and wait till they catch up. Next's app page and server-side component can work around it with use client, but the majority of client-side only packages will not be able to use this module and stuck with old version.

I'm now behind by 7 minors version... 😰

sammarks commented 1 year ago

I don't think removing the exports field will solve the issue in this case, though. Even if we remove the exports field, the default export is still going to be the main index file that loads all of the icons because the resolver isn't handling tree shaking properly.

For now, the solution (inside NextJS at least, if you're running into this issue) appears to be to just import each icon individually via iconoir-react/dist/CheckCircle or similar (from my comment above).

I know it's not ideal, but as far as I am aware our package is following what the NodeJS community expects it to follow.

I would also hate to make this change just for NextJS and end up with a worse experience for those that use other React frameworks.

louisgv commented 1 year ago

For now, the solution (inside NextJS at least, if you're running into this issue) appears to be to just import each icon individually via iconoir-react/dist/CheckCircle or similar (from my comment above).

When doing this, nextjs barf because they short circuit with the package.json's export entry xD... <- tho I have seen this behavior with most bundler as well esbuild, parcel, and even tsc when resolving ESM - if an exports field is specified, they short circuit the lookup entirely and respect just the package.json's export field. Thus, any path not explicitly specified in exports will not be counted...

As referenced in here https://github.com/iconoir-icons/iconoir/issues/243#issuecomment-1425395472 and the error again with latest icon-noir:

Module not found: Package path ./dist/esm/Link is not exported from package ...\node_modules\iconoir-react (see exports field in ...\node_modules\iconoir-react\package.json)

sammarks commented 1 year ago

Ah, yes, Typescript. I think the approach then should revolve around automatically generating a types.d.ts at the root of the package, in a way that both Typescript and NextJS are happy with explicit imports. I just haven't gotten around to it yet.

Mitsunee commented 1 year ago

I don't think removing the exports field will solve the issue in this case, though. Even if we remove the exports field, the default export is still going to be the main index file that loads all of the icons because the resolver isn't handling tree shaking properly.

For now, the solution (inside NextJS at least, if you're running into this issue) appears to be to just import each icon individually via iconoir-react/dist/CheckCircle or similar (from my comment above).

the issue with that is that the mere existence of the exports field entirely disables imports of individual files in Node.js (i.e import CheckCircle from "iconoir-react/dist/CheckCircle"). TypeScript of course seems to not understand this and will find types anyways, but not even Node's own resolver would find the actual cjs/mjs files, essentially breaking this workaround.

eckslol commented 1 year ago

Downgrading to v6.1.1 Worked for me

npm i iconoir-react@6.1.1 --force or npm i iconoir-react@6.1.1

as mention above by @louisgv

schardev commented 1 year ago

@Mitsunee Do you have an example of a lib using the exports approach successfully where the tree-shaking is working perfectly inside Next? That would be really helpful in updating our approach!

I'm using this icon lib along with iconoir and the tree shaking seems to be working fine (also as reported in #324, the modularizeImports option in next config works too).

amitshrivastavaa commented 5 months ago

Works by adding this in next.js config :

experimental: {
  optimizePackageImports: ['iconoir-react'],
}

iconoir-icons / iconoir