[pigment-css][react] throws errors in non-browser environment (i.e. unsafe when referenced) about material-ui

[brijeshb42]

Yeah. I'm thinking of doing something same here.

…

On Tue, 2 Apr 2024 at 6:14 PM, Flavien DELANGLE ***@***.***> wrote: @brijeshb42 <https://github.com/brijeshb42> in MUI X we tend to move everything that should not be imported inside an internals folder, that way importing it feels more wrong that utils. — Reply to this email directly, view it on GitHub <#41730 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFPF3VZQLKOJ2C7TAPY4ZLY3KRZLAVCNFSM6AAAAABFQW5472VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZRHE2DKNZVGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[brijeshb42]

Agreed. We'll have to clear these issues in our docs so that anything that goes into the config file should not be part of the code that goes into the source code. I've made a note of that.

[brijeshb42]

I wouldn't say it's a bug, rather a lack of documentation on how to export themes and styled/css function exports.
When you are using the same package to export styled/css calls as well as themes, either of those, preferably theme should be exported on a subpath and not in the same export as others.

In your case, you should be exposing your theme on @monorepo/pkg2/theme and other runtime exports can be directly on @monorepo/pkg2. This is becasue when trying to import @monorepo/pkg2 in your vite.config, it is indirectly also executing the css function which would throw error because it requires a code transform to work.
Hope this is clear. Let me know if this worked.

[o-alexandrov]

I disagree. The logic within Pigment's API is too fragile. css, keyframes, etc. all always throw an error when invoked. Instead, they should throw an error only in development and only in browser environment.
You are welcome to close the issue though, if you believe the DX is safe enough.
Anything cannot be safely imported from the file with a theme.

• if you import a value or even a type, you risk the file you import references a common file with a file that uses pigment

a lack of documentation on how to export themes

The issue has nothing to do with the documentation, it's about how the vite plugin fails (it says you didn't configure the bundler). At least the plugin should fail, stating there's a reference of a pigment file when importing a theme.

The simple solution would be to check on the existence of window, when Pigment's API is invoked.

• there could be a more complex check whether Pigment is invoked in browser or during bundling

[brijeshb42]

I am not sure where the fragility comes from. Importing a file in your bundler config is different than importing the same in your source code. Imports in source code go through transforms configured in the Vite config plugins. The same is not true for imports in config files.

When you are importing -

import { theme } from "../pkg2/src";

Vite executes the pkg2/src/index.ts file. This file also imports your pigment.css() definition. So this function gets executed as well. If you look at the css function definiton, it simply throws an error:

function css() {
  throw new Error(
    `@pigment-css/react: You were trying to call "css" function without configuring your bundler. Make sure to install the bundler specific plugin and use it. @pigment-css/vite-plugin for Vite integration or @pigment-css/nextjs-plugin for Next.js integration.`
  );
}

This is because it is supposed to be compiled away during bundling and get replaced with string class name. So importing and calling the same css function in your vite config file will throw the error since there are no code transforms involved.

This is the same reason that the extendTheme function is available through the @pigment-css/vite-plugin package and not from the @pigment-css/react package.

You can try this patch in your above repo. It just removes the theme export from the main index file.

commit a75173dd1270ae1497b295e5f7be9b9652805ee5
Author: Brijesh Bittu <[email protected]>
Date:   Mon Apr 1 13:52:37 2024 +0530

    Subpath export

diff --git a/pkg1/vite.config.ts b/pkg1/vite.config.ts
index b8a532f..e48eece 100644
--- a/pkg1/vite.config.ts
+++ b/pkg1/vite.config.ts
@@ -2,7 +2,7 @@ import { defineConfig } from "vite";
 
 import react from "@vitejs/plugin-react";
 import { pigment, extendTheme } from "@pigment-css/vite-plugin";
-import { theme } from "../pkg2/src";
+import { theme } from "../pkg2/src/theme";
 
 export default defineConfig(() => ({
   plugins: [
diff --git a/pkg2/src/index.ts b/pkg2/src/index.ts
index 514cada..6f6427c 100644
--- a/pkg2/src/index.ts
+++ b/pkg2/src/index.ts
@@ -1,2 +1 @@
 export * as css from "./css";
-export * from "./theme";

[o-alexandrov]

I just provided you with the simplest possible example to highlight the issue.
In the real apps, the references might happen through several files and it could start with an import of a type (it happened to me when I tried to use Pigment in a big repo).

Please refer to the proposed solution in my previous reply.

The logic within Pigment's API is too fragile. css, keyframes, etc. all always throw an error when invoked. Instead, they should throw an error only in development and only in browser environment.

The simple solution would be to check on the existence of window, when Pigment's API is invoked.

• there could be a more complex check whether Pigment is invoked in browser or during bundling

[brijeshb42]

Also, though this import is available @pigment-css/react/utils and I see that you are using it in your code, but it's not documented. It's only for sharing logic internally across next/vite plugin packages.
So anything not documented should not be used in your source code and should be always be considered private and can be break at any point.

[flaviendelangle]

@brijeshb42 in MUI X we tend to move everything that should not be imported inside an internals folder, that way importing it feels more wrong that utils.

[flaviendelangle]

With that being said, I do agree that we should be crystal clear about what can be imported in a config file 👍

[o-alexandrov]

This issue has nothing to do with internals; and it's not a question issue.
I mentioned the same thing in the previous 2 replies, here's the same summary.

The logic within Pigment's API is too fragile. css, keyframes, etc. all always throw an error when invoked. Instead, they should throw an error only in development and only in browser environment.

The simple solution would be to check on the existence of window, when Pigment's API is invoked.

• there could be a more complex check whether Pigment is invoked in browser or during bundling

[flaviendelangle]

Could you please describe the scenario were you have a file that contains css or keyframe and which is executed in an environment which causes an unwanted error?

[brijeshb42]

The scenario is outlined in the linked repo in the first comment.

The main issue is that one of the packages is exporting both styled/css function calls as well as a theme object. The error arises from the fact that calling styled/css directly (for ex: similar to importing and calling them in the node REPL) will directly throw an error. They are supposed to be transformed through bundler plugin and compiled away.
In this case, since the package is also being imported in the vite config, there's no code transformation happening and css is also being called which is throwing an error. It's by design. So my main proposal was that the theme (which will be used in the config) be exported through a subpath so that no css call happens there.

It's not just about monorepo as stated. It can happen in a standalone npm package as well.

[o-alexandrov]

Here's how vite treats a similar issue, TLDR as a bug.