tabler / tabler-icons

A set of over 5700 free MIT-licensed high-quality SVG icons for you to use in your web projects.
https://tabler.io/icons
MIT License
18.36k stars 921 forks source link

Don't bundle the CJS build #471

Open anthonyalayo opened 1 year ago

anthonyalayo commented 1 year ago

Follow up to https://github.com/tabler/tabler-icons/issues/359

Adding support for consistent webpack import transforms has been great for performance, thank you!

However with Next.js, server side builds are still built with CJS. The module system with Node is still in its infancy and hasn't yet been widely adopted.

Utilizing modularizeImports like so:

  modularizeImports: {
    '@tabler/icons-react': {
      transform: '@tabler/icons-react/dist/esm/icons/{{member}}',
    },
  },

Results in the following error:

import createReactComponent from '../createReactComponent.js';
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at internalCompileFunction (node:internal/vm:74:18)
    at wrapSafe (node:internal/modules/cjs/loader:1141:20)
    at Module._compile (node:internal/modules/cjs/loader:1182:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1272:10)
    at Module.load (node:internal/modules/cjs/loader:1081:32)
    at Module._load (node:internal/modules/cjs/loader:922:12)
    at Module.require (node:internal/modules/cjs/loader:1105:19)
    at require (node:internal/modules/cjs/helpers:103:18)

This can be fixed by configuring @tabler/icons-react to be transpiled as well:

  transpilePackages: ['@tabler/icons-react'],

But this is more of a workaround until the library is being emitted in a better format.

Can we update the CJS build to not be bundled? @mui/icons-material came to the same conclusion, and that's how their emitting their CJS builds.

codecalm commented 1 year ago

that is, if I understand correctly, the cjs package and the entries about it in package.json are unnecessary ?

anthonyalayo commented 1 year ago

@codecalm no changes needed in the package.json for this one.

The Short Answer: We should make the CJS build like the ESM build -- not bundled into one file.

If you pull @mui/icons-material, you can get a demo of this.

The Long Answer: Node has CJS as the default module format, but modern browsers all support ESM now. Using an import transform plugin like the one for Next.js / SWC with tabler icons is currently working with ESM, but not working with CJS. Both need to be supported these days because of the popularity of Next.js / server side rendering. The client side bundle it produces has no issues with ESM but the server side bundle does.

I believe the future will be all about emitting .mjs files and setting up conditional imports in your package.json, but we arent there yet. I again used material-icons as a reference here to see if they checked off those boxes yet (which they haven't).

AlonMiz commented 1 year ago

yes pls 🙏 i upgraded the package to version 2 on my repo, hopefully to decrease bundle size, but the bundle size gain more weight :/ BTW, can we have some kind of a workaround meanwhile? something like this that's doesnt really work. just an example

import IconBrandInstagram from '@tabler/icons-react/dist/esm/icons/IconBrandInstagram';
anthonyalayo commented 1 year ago

@codecalm this should be an easy fix by just not bundling the CSM build

codecalm commented 1 year ago

@anthonyalayo so what should be in "main": "dist/cjs/tabler-icons-react.js", line when cjs will be not builded?

anthonyalayo commented 1 year ago

@codecalm so that can stay the same, here's what I'm thinking:

  1. We have our source code for the project (most likely the latest and greatest JS)
  2. The build outputs a CJS folder (which has module.exports) and an ESM folder (which has imports)
  3. Neither of those folders are bundled, so they only contain the transpiled code in their individual files
  4. The main/module fields in the package.json would simply point to those same entry points (ie. tabler-icons-react.js)

Here's a blog that talks about this approach in detail with an example: https://cmdcolin.github.io/posts/2022-05-27-youmaynotneedabundler#why-would-you-not-want-a-bundler-for-your-library

mmahalwy commented 1 year ago

@anthonyalayo any update on this? :)

anthonyalayo commented 1 year ago

@mmahalwy I haven't heard from @codecalm since February

codecalm commented 1 year ago

I am alone in the project, unfortunately I do not have time to do all the issues. If anyone has an idea and willingness to do PR, please feel free to contribute 🙂

oliverkidd commented 1 year ago

Is there going to be any progress here or is this the end of the line for using tabler icons in nextjs13?

hensansi commented 1 year ago

I was looking into it today too, would something along these line solve this? (no breaking changes but see note below) https://github.com/tabler/tabler-icons/compare/master...hensansi:tabler-icons:modularize-commonjs-components

@codecalm @anthonyalayo

note: I see on the config that es <-> esm are the same and this is being done so the generated file isn't overwritten. My solution does something along those lines too so it doesn't introduce breaking changes but eventually it would make sense to prefix the bundled/single files with bundle even though I don't know what are the conventions this kind of library

AlonMiz commented 1 year ago

Next js added automatic optimization to some icon libraries but, tabler isn't mentioned there https://nextjs.org/blog/next-13-5#optimized-package-imports

image