Skip to content
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

Culori ESM import throws an error in Next.js production build #195

Closed
VojtechVidra opened this issue Mar 12, 2023 · 5 comments
Closed

Culori ESM import throws an error in Next.js production build #195

VojtechVidra opened this issue Mar 12, 2023 · 5 comments
Labels
Bug Something isn't working

Comments

@VojtechVidra
Copy link
Contributor

Hi Dan Burzo!

I've encountered strange issue when using latest culori with Next.js in production build only. I've created minimal reproduction of the issue (see the repo below).

How to reproduce

Dev

If you run the app with npm run dev command it works as expected, the converter converts the color with no problem in both the culori and culori/require usage.

Prod

However if you run the production build npm run build && npm run start the culori ESM version throws an error but the culori/require version still works as expected.

https://github.com/VojtechVidra/nextjs-culori

I'm not sure if this is an issue with Culori or Next.js, but I've figured I'll ask you first.

Thanks for creating this great package!

@danburzo
Copy link
Collaborator

danburzo commented Mar 12, 2023

Hi @VojtechVidra! What seems to be going on is some unintended tree-shaking of the ESM entry point src/index.js when you import something that the bundler deems to be free of side-effects.

Importing just converter seems to skip all the side-effects further down in the file:

import { converter } from "culori";

If instead you import the LCh converter directly, which is defined as a side-effect in the entry point, all the other side effects are executed and the code works:

import { lch } from 'culori';

If I understand correctly, Next.js 13 uses Turbopack, so that may be the place to open up an issue. I misread that, Next.js 13 uses a built-in Compiler, so the Next.js repo may be the place to log the issue. It feels to me like overeager tree-shaking. Admittedly, the way Culori is organized is a bit quirky!

This is not a problem for the CommonJS bundle because tree-shaking is not applicable there.

@danburzo danburzo changed the title Culori throws an error in Next.js production build Culori ESM import throws an error in Next.js production build Mar 12, 2023
@danburzo
Copy link
Collaborator

The issue may be caused by our usage of sideEffects: false; in package.json, which if I remember correctly was necessary in order to allow for tree-shaking ESM modules. I'm going to leave the issue open as we may want to revisit this value in the future, to offer better support for the various bundlers.

@danburzo danburzo added the Bug Something isn't working label Mar 12, 2023
@marcelpi
Copy link

marcelpi commented Jun 2, 2023

The issue may be caused by our usage of sideEffects: false; in package.json, which if I remember correctly was necessary in order to allow for tree-shaking ESM modules. I'm going to leave the issue open as we may want to revisit this value in the future, to offer better support for the various bundlers.

I confirm this.

Same thing happens when creating a build with Svelte.
Local dev environment works as expected, whereas when creating a build I get some sort of error which traces back to the interpolate function.

I manually removed "sideEffects": false, from the package.json of culori and the build works fine afterwards.

@danburzo
Copy link
Collaborator

danburzo commented Jun 2, 2023

In culori@3.1.2 I've replaced sideEffects: false with an array of files producing side effects. This should allow both the tree-shaken version (using culori/fn) and the non-tree-shaken version to work correctly with more bundlers.

Tested the files in test/tree-shaking/ with esbuild, rollup, and parcel. Let me know how this new setup works for you!

@marcelpi
Copy link

marcelpi commented Jun 2, 2023

I just updated to 3.1.2 and can confirm the build is working as expected in a Svelte app.
Thanks @danburzo , you rock dude.

@danburzo danburzo closed this as completed Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants