-
-
Notifications
You must be signed in to change notification settings - Fork 173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUG: Too many open files on Next 13 #243
Comments
Note: I'm not using the |
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 -- 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. |
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. |
I'm actually seeing the same issue on my server, even with importing files individually, like so:
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). |
More context, here're some sample of me importing the icons:
|
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 |
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' |
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:
|
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. |
Hmm, it seems to me that because of the |
Yup - which is indeed the case. That's the key diff that mattered between 6.1.1 vs 6.2.0 - the |
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. |
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): |
@Mitsunee Do you have an example of a lib using the |
@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 {
"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 |
@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 |
@sammarks yes, this approach does work for Node.js, but TypesSript is currently not able to use this 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 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. |
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. |
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 I'm now behind by 7 minors version... 😰 |
I don't think removing the 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 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. |
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 #243 (comment) and the error again with latest icon-noir:
|
Ah, yes, Typescript. I think the approach then should revolve around automatically generating a |
the issue with that is that the mere existence of the exports field entirely disables imports of individual files in Node.js (i.e |
Downgrading to v6.1.1 Worked for me
as mention above by @louisgv |
I'm using this icon lib along with |
Works by adding this in next.js config :
|
Prerequisites
Step to reproduce
Actual behavior
NOTE: Dev/Build is working just fine on the local machine
Any message or error
Additional info or screenshots
Downgrading to 6.1.0 fixed the issue. This is likely introduced by #240
Upvote & Fund
The text was updated successfully, but these errors were encountered: