vanilla-extract-css / vanilla-extract

Zero-runtime Stylesheets-in-TypeScript
https://vanilla-extract.style
MIT License
9.64k stars 295 forks source link

errors when using external ui library implemented with vanilla-extract in next.js app #940

Open jneander opened 1 year ago

jneander commented 1 year ago

Describe the bug

In a Next.js app using the @vanilla-extract/next-plugin to support VE locally, everything works as expected. However, when trying to use view code from an external library that has styles implemented with vanilla-extract, there are render errors.

The minimal example for can be found here:

The following behavior is observed while the Next.js app is running in dev mode:

A. The external code is not used on the page.

  1. Refresh the page.
  2. Observe that the page loads successfully.

B. The external code is added to the page.

  1. Observe that the page injects the external code to the page successfully.
  2. Refresh the page.
  3. Observe that the page displays the error described below.
Server Error
Error: Styles were unable to be assigned to a file. This is generally caused by one of the following:

- You may have created styles outside of a '.css.ts' context
- You may have incorrect configuration. See https://vanilla-extract.style/documentation/getting-started

Reproduction

https://github.com/jneander/example-app-using-third-party-vanilla-extract-ui-library

System Info

System:
  OS: macOS 13.0
  CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
  Memory: 359.04 MB / 16.00 GB
  Shell: 3.2.57 - /bin/bash
Binaries:
  Node: 18.12.1 - ~/.nvm/versions/node/v18.12.1/bin/node
  Yarn: 1.22.17 - /usr/local/bin/yarn
  npm: 8.19.2 - ~/.nvm/versions/node/v18.12.1/bin/npm
Browsers:
  Chrome: 108.0.5359.94
  Firefox: 97.0.2
  Safari: 16.1
npmPackages:
  @vanilla-extract/css: ^1.9.2 => 1.9.2 
  @vanilla-extract/next-plugin: ^2.1.1 => 2.1.1

Used Package Manager

npm

Logs

Server Error
Error: Styles were unable to be assigned to a file. This is generally caused by one of the following:

- You may have created styles outside of a '.css.ts' context
- You may have incorrect configuration. See https://vanilla-extract.style/documentation/getting-started

This error happened while generating the page. Any console logs will be displayed in the terminal window.
Call Stack
Object.getFileScope
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/@vanilla-extract/css/fileScope/dist/vanilla-extract-css-fileScope.cjs.dev.js (33:11)
generateIdentifier
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/@vanilla-extract/css/dist/vanilla-extract-css.cjs.dev.js (183:49)
style
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/@vanilla-extract/css/dist/vanilla-extract-css.cjs.dev.js (404:19)
Object.<anonymous>
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/@jneander/x-throwaway-vanilla-extract-ui/dist/cjs/example.css.js (5:32)
Module._compile
node:internal/modules/cjs/loader (1159:14)
Module._extensions..js
node:internal/modules/cjs/loader (1213:10)
Module.load
node:internal/modules/cjs/loader (1037:32)
Module._load
node:internal/modules/cjs/loader (878:12)
Module.require
node:internal/modules/cjs/loader (1061:19)
require
node:internal/modules/cjs/helpers (103:18)
Object.<anonymous>
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/@jneander/x-throwaway-vanilla-extract-ui/dist/cjs/example.js (5:23)
Module._compile
node:internal/modules/cjs/loader (1159:14)
Module._extensions..js
node:internal/modules/cjs/loader (1213:10)
Module.load
node:internal/modules/cjs/loader (1037:32)
Module._load
node:internal/modules/cjs/loader (878:12)
Module.require
node:internal/modules/cjs/loader (1061:19)
require
node:internal/modules/cjs/helpers (103:18)
Object.<anonymous>
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/@jneander/x-throwaway-vanilla-extract-ui/dist/cjs/index.js (17:14)
Module._compile
node:internal/modules/cjs/loader (1159:14)
Module._extensions..js
node:internal/modules/cjs/loader (1213:10)
Module.load
node:internal/modules/cjs/loader (1037:32)
Module._load
node:internal/modules/cjs/loader (878:12)
Module.require
node:internal/modules/cjs/loader (1061:19)
require
node:internal/modules/cjs/helpers (103:18)
@jneander/x-throwaway-vanilla-extract-ui
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/.next/server/pages/index.js (54:18)
__webpack_require__
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/.next/server/webpack-runtime.js (33:42)
eval
webpack-internal:///./pages/index.tsx (8:98)
./pages/index.tsx
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/.next/server/pages/index.js (43:1)
__webpack_require__
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/.next/server/webpack-runtime.js (33:42)
__webpack_exec__
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/.next/server/pages/index.js (75:39)
<unknown>
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/.next/server/pages/index.js (76:28)
Object.<anonymous>
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/.next/server/pages/index.js (79:3)
Module._compile
node:internal/modules/cjs/loader (1159:14)
Module._extensions..js
node:internal/modules/cjs/loader (1213:10)
Module.load
node:internal/modules/cjs/loader (1037:32)
Module._load
node:internal/modules/cjs/loader (878:12)
Module.require
node:internal/modules/cjs/loader (1061:19)
require
node:internal/modules/cjs/helpers (103:18)
Object.requirePage
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/next/dist/server/require.js (88:12)
<unknown>
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/next/dist/server/load-components.js (37:73)
async Object.loadComponents
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/next/dist/server/load-components.js (37:26)
async DevServer.findPageComponents
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/next/dist/server/next-server.js (548:36)
async DevServer.renderPageComponent
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/next/dist/server/base-server.js (926:24)
async DevServer.renderToResponse
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/next/dist/server/base-server.js (955:32)
async DevServer.pipe
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/next/dist/server/base-server.js (406:25)
async Object.fn
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/next/dist/server/next-server.js (744:21)
async Router.execute
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/next/dist/server/router.js (252:36)
async DevServer.run
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/next/dist/server/base-server.js (383:29)
async DevServer.run
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/next/dist/server/dev/next-dev-server.js (734:20)
async DevServer.handleRequest
file:///Users/jeremy/Projects/temp/example-app-with-ve-bug/node_modules/next/dist/server/base-server.js (321:20)

Validations

askoufis commented 1 year ago

Hi, thanks for the reproduction.

Your app looks fine to me. I think the issue might be how your UI library is bundled. You'll need to use one of the bundler integrations to create a package that can be consumed correctly in a project that uses vanilla-extract.

Take a look at the webpack or the rollup integrations for some ideas of how to get that working. Alternatively, a discord user recently shared an example repo that uses rollup to bundle a UI package that uses vanilla-extract for styling. https://github.com/graup/vanilla-extract-rollup-example/tree/main/packages/ui.

Feel free to join the discord for some quicker feedback from community members if you need some help getting things working.

jneander commented 1 year ago

Thank you for the response, Adam. Per your suggestion, I've joined the Discord and have asked about this issue. Still awaiting a response there.

My assumption with bundling on the library side is that it would result in outputting .css assets. My intention is to preserve the vanilla-extract implementation and carry the responsibility of converting that over to the consumer side. The consumer would need to know how to handle VE source, but would have complete control over how the source is consumed and the output distributed.

To help with picking apart the example, I've pushed the example library to a repo, included its dist in the source, and have updated the library link to point to the repo instead of the npmjs.com page.

I'd love to figure out how to get this working. Hopefully, it's just something I'm overlooking in my implementation.

jneander commented 1 year ago

I'm still stumped by this. I've been trying to sort it out without diving deep into unfamiliar internals of VE, hoping it's something obviously wrong that I'm doing. Based on prior conversations, I'm curious about whether or not VE expects .css.ts files from the external library, or if it also understands .css.js files in that library providing the same, but de-typed code.

The fact that it works some of the time ought to be a clue. It suggests that this approach should work, but is missing something necessary for VE to handle it properly in all cases. 😕

jneander commented 1 year ago

I stumbled onto #620. After reading through it and experimenting with the example and its approach, it seems like the only way to use VE in the manner tried in this issue is to eliminate all code transformation on the library prior to publishing, releasing the original source—e.g..ts, .css.ts, .tsx), then transpiling it on the application side (via next-transpile-modules for this Next.js example) and passing it through the appropriate VE build plugin.

For a small project, this might be feasible. But for larger project with several intermediate (and very legacy) libraries, this means that every library adopting VE would need to publish its original source for this approach to work. That is, of course, assuming that this conclusion is accurate.

As an additional quirk, when transpiling the module on the Next.js side but converting .ts to .js before library publication, the following behavior is observed:

A. The external code is not used on the page.

  1. Refresh the page.
  2. Observe that the page loads successfully.

B. The external code is added to the page.

  1. Observe that the page injects the external code to the page successfully.
  2. Refresh the page.
  3. Observe that the page displays the markup, but not using the styles from the library.
  4. Open the dev tools console and observe that the following warning has been printed:
next-dev.js?3515:20 Warning: Prop `className` did not match. Server: "null" Client: "example_huge__st42y10"
    at div
    at Example (webpack-internal:///../x-throwaway-vanilla-extract-ui/dist/esm/example.js:11:11)
    at div
    at Home
    at MyApp (webpack-internal:///./pages/_app.tsx:9:11)
    at PathnameContextProviderAdapter (webpack-internal:///./node_modules/next/dist/shared/lib/router/adapters.js:62:11)
    at ErrorBoundary (webpack-internal:///./node_modules/next/dist/compiled/@next/react-dev-overlay/dist/client.js:301:63)
    at ReactDevOverlay (webpack-internal:///./node_modules/next/dist/compiled/@next/react-dev-overlay/dist/client.js:850:919)
    at Container (webpack-internal:///./node_modules/next/dist/client/index.js:60:1)
    at AppContainer (webpack-internal:///./node_modules/next/dist/client/index.js:173:11)
    at Root (webpack-internal:///./node_modules/next/dist/client/index.js:346:11) 

See more info here: https://nextjs.org/docs/messages/react-hydration-error
askoufis commented 1 year ago

Hi @jneander. Sorry for the delayed response.

In my original comment I thought you were trying to achieve something different, but given the context you've provided I think I now understand the problem better.

As far as I know, there's no reason your approach shouldn't work. Vanilla extract integrations should find files ending in .css.ts OR .css.js. The example repo linked in the issue you referenced transpiles its files to javascript, and those files are consumed just fine in the app adjacent to the components library. https://github.com/mihkeleidast/vanilla-extract-component-library-example

In fact, my team at SEEK is working on a packaging tool that works by this principle, as our design system has gotten large enough that it has warranted investment in speeding up build times. Currently we just ship raw typescript and put the onus on the consumer to transpile it in their app, which as you've mentioned, doesn't scale very well.

The error you mentioned in the top-level issue comment is coming from here, which is called slightly further down the call stack from the style function in your css.js file. The webpack plugin should be inserting a setFileScope call before style is called, which would prevent that error from being thrown.

Debugging your app, I noticed that the VE webpack plugin was transforming the esm version of example.css.js, and then the app was loading the untransformed cjs version of example.css.js, causing the error. Why this is happening I'm not so sure about, but that seems to be the crux of the issue. This could be something to do with the component library's package.json, maybe an exports key could do the trick. I'm not super knowledgeable about ESM resolution, but I do know it has some weird quirks that can lead to unexpected results.

Hope this helps.

jneander commented 1 year ago

Hey @askoufis. No worries about the delay.

Apologies for not being more clear about what I was trying to accomplish in the original post. Your mention of the cjs/esm mismatch was a solid clue. I just tried to switch to using the exports field in package.json. But everything went haywire. After that, I just dropped the cjs path from the dist. I figured it can't mismatch them when there's only one, right?

Lo and behold, it worked!

This warrants further debugging, as there seems to be something allowing the mismatch to happen. If the easy fix of dropping cjs holds up, then I might have a path forward for now. But for folks needing both esm and cjs for legacy purposes, this little gremlin might be a showstopper. I'll dig deeper and see what I come up with. It's as good a time as any to learn more about the VE internals.

uglow commented 1 year ago

I came across this issue after having the same problem. First some background...

I've been using Jest + Rush for a TS -> ESM-module monorepo (multiple packages). Each package has `"type": "module", with an export prop in package.json. To get Jest to correctly load (without errors) a local-package that used VE, I needed this config in my NextJS app (jest.config.cjs)

For Jest

const commonConfig = require('@eme-tools/jest');
const nextJest = require('next/jest');

const createJestConfig = nextJest({
  // Provide the path to your Next.js app to load next.config.js and .env files in your test environment
  dir: './',
});

// Add any custom config to be passed to Jest
const customJestConfig = {
  ...commonConfig,

  moduleNameMapper: {
    'next/config': '<rootDir>/test/__mocks__/getConfig.ts', // For some reason, ESM-jest says the default export from next/config is not a function.
    'next/head': '<rootDir>/test/__mocks__/head.ts', // For some reason, ESM-jest says that next/head returns an object instead of a function/class/string
  },
  testEnvironment: 'jest-environment-jsdom',
  testMatch: ['<rootDir>/src/**/*.spec.{ts,tsx}'],

  // The following config is required to use Jest in ESM mode.
  // It is the same config that the library and api packages use.
  globals: {
    'ts-jest': {
      tsconfig: 'tsconfig.json',
      isolatedModules: true, // Without this flag, the test hangs. Disables type checking --> faster!
      useESM: true,
      // This Babel config is for vanilla-extract only
      babelConfig: {
        presets: ['next/babel'],
        plugins: ['@vanilla-extract/babel-plugin'],
      },
    },
  },
  globalSetup: '<rootDir>/../../tools/jest/globalSetup.cjs',
  preset: 'ts-jest/presets/default-esm', // Required
};

// createJestConfig is exported this way to ensure that next/jest can load the Next.js config which is async
module.exports = async () => {
  const config = await createJestConfig(customJestConfig)();

  // ------------------- IMPORTANT BIT BELOW --------------------
  // vanilla-extract: See https://vanilla-extract.style/documentation/setup/#test-environments
  delete config.moduleNameMapper['^.+\\.(css|sass|scss)$'];
  // Use ts-jest for css.ts files. This allows us to use vanilla-extract with jest esm / ts-jest/ babel
  config.transform = { '^.+\\.css.ts$': 'ts-jest', ...config.transform };
  // This line is necessary for the compiled css.ts files in web-common to be parsed without issue.
  config.transform = { '^.+\\.css.js$': 'ts-jest', ...config.transform };
  // ------------------- END --------------------

  return config;
};

After attempting to switch to Vitest, I did the usual find-and-replace jest with vi, and got most of the tests working. However, VE started to complain again with the error in this issue. Armed with a working Jest config, I was finally able to come up with this solution (vitest.config.ts):

For Vitest:

/// <reference types="vitest" />
import { resolve } from 'path';
import { vanillaExtractPlugin } from '@vanilla-extract/vite-plugin';
import react from '@vitejs/plugin-react';
import { defineConfig } from 'vitest/config';

// https://vitejs.dev/config/
export default defineConfig({
  plugins: [
    react({
      babel: {
        plugins: ['@vanilla-extract/babel-plugin'],
      },
    }),
    vanillaExtractPlugin(),
  ],
  test: {
    deps: {
      inline: true, // Required in my case, maybe not in yours
    },
    environment: 'jsdom',
    include: ['src/**/*.spec.ts'],
    exclude: ['**/node_modules/**', 'fixtures/**/*'],
    globals: true,

    // ----------------- IMPORTANT BIT ----------------------
    transformMode: {
      web: [/\.css.js$/], // This is the key line that tells Vitest how to handle pre-compiled Vanilla-Extract code.
    },
    // ----------------- END ----------------------
  },
});

Note that the child-dependency is only compiled (not bundled) using plain ol' TypeScript. The VE docs assume that you would use ESbuild or Rollup, which isn't always the case (especially in monorepos with library packages). Or should I be bundling internal-library packages?

smitev commented 1 year ago

Hey @askoufis. No worries about the delay.

This warrants further debugging, as there seems to be something allowing the mismatch to happen. If the easy fix of dropping cjs holds up, then I might have a path forward for now. But for folks needing both esm and cjs for legacy purposes, this little gremlin might be a showstopper. I'll dig deeper and see what I come up with. It's as good a time as any to learn more about the VE internals.

Any new findings on this @jneander ? I have the same case as you, UI library that exports VE and I need to support both esm and cjs.

viclafouch commented 1 year ago

Hello !

So if I understand correctly, we can't use properly Vite.js with VE for a library ?

Because, as I see in my application, I have to import the style.css file bundled (by my library package) that includes all styles of my components... (even not rendered..) :/

I really want to use Vite.JS (ESM first).

jd-oconnor commented 1 year ago

Hey @askoufis. No worries about the delay. This warrants further debugging, as there seems to be something allowing the mismatch to happen. If the easy fix of dropping cjs holds up, then I might have a path forward for now. But for folks needing both esm and cjs for legacy purposes, this little gremlin might be a showstopper. I'll dig deeper and see what I come up with. It's as good a time as any to learn more about the VE internals.

Any new findings on this @jneander ? I have the same case as you, UI library that exports VE and I need to support both esm and cjs.

If you start a thread in discord, I can help you out. I built a UI library with VE and am using it at work.

jd-oconnor commented 1 year ago

Hello !

So if I understand correctly, we can't use properly Vite.js with VE for a library ?

Because, as I see in my application, I have to import the style.css file bundled (by my library package) that includes all styles of my components... (even not rendered..) :/

I really want to use Vite.JS (ESM first).

You can use it. They have a plug-in for integrating with vite. Message us on discord if you get stuck

https://vanilla-extract.style/documentation/integrations/vite/

smitev commented 1 year ago

@viclafouch we had some challenges with VE UI Library to integrate it with Vite. With some tweaking in the Vite config, we succeed to make it work. The hack was to exclude the UI library packages from the "optimizeDeps".

Dep Optimization Options

This is how our vite.config.ts looks now:

import { vanillaExtractPlugin } from "@vanilla-extract/vite-plugin";
import react from "@vitejs/plugin-react-swc";
import { defineConfig } from "vite";

export default defineConfig({
  plugins: [react(), vanillaExtractPlugin()],
  optimizeDeps: {
    exclude: [
      "@filamix-ui",
      "@filamix-ui/generics",
    ],
  },
});

Worth mentioning is that from "@filamix-ui" we are exporting subpaths thru package.json. It works fine. Just make sure in your tsconfig.json to set "compilerOptions" > "moduleResolution" to "Node16".

smitev commented 1 year ago

@jd-oconnor

If you start a thread in discord, I can help you out. I built a UI library with VE and am using it at work.

Are you using it with Next.js?

jd-oconnor commented 1 year ago

@jd-oconnor

If you start a thread in discord, I can help you out. I built a UI library with VE and am using it at work.

Are you using it with Next.js?

I’m not. I can spin up a couple test apps with next 12 and next 13 to see if they work and let you know.

roginfarrer commented 1 year ago

This warrants further debugging, as there seems to be something allowing the mismatch to happen. If the easy fix of dropping cjs holds up, then I might have a path forward for now. But for folks needing both esm and cjs for legacy purposes, this little gremlin might be a showstopper. I'll dig deeper and see what I come up with. It's as good a time as any to learn more about the VE internals.

I think I've been running into the same thing (with just the Webpack plugin). For me, it boiled down to the fact that my component library is published in CJS, and I don't believe that the Vanilla Extract tooling supports CommonJS. I think I have a fix in #970.

In the meantime, I found that the workaround is to include the @vanilla-extract/babel-plugin (now deprecated) in the library build step. It appears to inject the addFileScope stuff prior to the file being converted to ESM. So now my published Button.css.js file looks something like this:

Object.defineProperty(exports, "__esModule", {
  value: true
});

var __vanilla_filescope__ = _interopRequireWildcard(require("@vanilla-extract/css/fileScope"));

var _css = require("@vanilla-extract/css");

__vanilla_filescope__.setFileScope("src/Button/Button.css.ts", "my-library");

var button = _css.style({});

exports.button = button;

__vanilla_filescope__.endFileScope();

Then when the app consumes, this file, @vanilla-extract/integration doesn't need to care if the file is CJS or ESM. See here.

I don't think this is the ideal solution by any means, but it's a decent work around, and I think this can be handled upstream (maybe my PR is the right solution, maybe not).

otaviomad commented 1 year ago

Both the workaround and fix don't work on my next js setup. This is a huge blocker for us :/

Update:

After updating the packages and moving some things around, I've managed to get it to work... However the classes being imported seem to differ between server and client. The class that gets served in the plain HTML is the CJS version and the version the client loads is the ESM version. There's a mismatch in the classNames.

After some digging around, I found this issue:

https://github.com/vercel/next.js/issues/35581

I believe this error might be on Next.js' end, but maybe there's a way to work around it on the webpack plugin?

cseas commented 1 year ago

It looks like this is more of a feature request than a bug. Shipping styles without compilation isn't really how Vanilla Extract is meant to be used. As a UI library author, I don't see the benefit in forcing every user to compile the VE styles themselves from node_modules.

My component library is working without issues in Next.js v13.1.2 app directory. For reference, here's the library source code and usage in a Next.js app.

For anyone getting blocked by this, the solution might just be to add the respective bundler plugin to your library so you're shipping static styles, not TypeScript. And if there are any significant benefits to shipping TypeScript and making users compile VE styles, I'd love to see a docs page on this.

otaviomad commented 1 year ago

It looks like this is more of a feature request than a bug. Shipping styles without compilation isn't really how Vanilla Extract is meant to be used. As a UI library author, I don't see the benefit in forcing every user to compile the VE styles themselves from node_modules.

I disagree. I don't see why functionality should differ because the styles come from node_modules? It's not that complex.

For anyone getting blocked by this, the solution might just be to add the respective bundler plugin to your library so you're shipping static styles, not TypeScript.

That is, of course, what we are already doing. Setting up Vanilla Extract is a requirement for using our styles.

And if there are any significant benefits to shipping TypeScript and making users compile VE styles, I'd love to see a docs page on this.

We're decoupling every single component style. Each component style is a package that can be built individually. We have a pretty big UI Toolkit. This means that when we update a small style, only that specific package needs to be updated and rebuilt, which would not be the case if we pre-built our vanilla extract files into CSS, as that would potentially mess up the variable scoping.

cseas commented 1 year ago

It's not that complex.

It gets complex in the context of Next.js because it doesn't transpile node_modules out of the box. And I'm not sure relying on Next.js to transpile your library is the optimal solution because there's no documentation on how that transpilation works and what's the config used. I've always seen next-transpile-modules config being used as a silver bullet to fix broken packages, it's not the happy path most devs expect with libs.

next-transpile-modules has been historically used for 2 use-cases:

  1. Local libraries in monorepos
  2. NPM packages that use ES6 imports

A published library is very different from the first use-case. And Next.js supports ESM libraries now so the second use case for a published package is redundant now.

Each component style is a package that can be built individually.

Is that not possible with VE currently? Each component gets its own CSS file after transpilation so we can update styles of one component without affecting others. Variable scoping shouldn't be a problem as all bundlers aim for deterministic builds. So if you run a build twice on the same code, it shouldn't mean that you get different class names each time. I'd assume VE's hashing algorithm is deterministic too, it's a bug if it isn't.

Can you please elaborate a bit more? I'm really not seeing the benefit in moving transpilation to consumer projects. All it'll achieve is make installation of your UI toolkit harder for users.

otaviomad commented 1 year ago

It gets complex in the context of Next.js because it doesn't transpile node_modules out of the box.

As I stated in a previous edit, I think this is a problem on Next.js' part. Just by looking at the classNames generated, I can see that the server is serving from the cjs module while the client is importing from the esm module. There is a stale issue about this on their github (https://github.com/vercel/next.js/issues/35581). The workaround is deleting the "module" key from our package.json.

Is that not possible with VE currently? Each component gets its own CSS file after transpilation so we can update styles of one component without affecting others. Variable scoping shouldn't be a problem as all bundlers aim for deterministic builds. So if you run a build twice on the same code, it shouldn't mean that you get different class names each time. I'd assume VE's hashing algorithm is deterministic too, it's a bug if it isn't.

Can you please elaborate a bit more? I'm really not seeing the benefit in moving transpilation to consumer projects. All it'll achieve is make installation of your UI toolkit harder for users.

Sure! It indeed is possible, however as I said, you may end up running into scoping issues. We extensively use contracts for our values, and there is one main theme which gets consistently referenced by all our components. If at some point, someone decides to make a small change to the main theme, it means that only the theme package needs to be updated. This would not be the case with pre-built CSS, since building the theme package would generate a new hash for the variable scope and thus all the CSS that references that would have to be rebuilt, even if they weren't changed at all. Since all of our contracts also come from the same place, it would mean that adding a new component would have to trigger a new version for all the components as well, which pretty much just invalidates the reasoning for having separate component versions.

otaviomad commented 1 year ago

I think I found a definitive fix. Make this change to your next.config.js:

/** @type {import('next').NextConfig} */
const nextConfig = {
  reactStrictMode: true,
  webpack(config, options) {
    config.module.rules.unshift({
      test: /@your-theme-scope/, // Can also be your individual package name
      resolve: {
        mainFields: ['module'],
      },
    });

    return config;
  },
};

This will make it so that the imports are consistent between client and server. You also need to write an exports field in your package as well as your main and module fields (which I thought was pretty weird but it consistently fixed the issue):

{
  "main": "./dist/cjs/index.css.js",
  "module": "./dist/esm/index.css.js",
  "types": "./dist/types/index.css.d.ts",
  "exports": {
    ".": {
      "import": "./dist/esm/index.css.js",
      "require": "./dist/cjs/index.css.js",
      "types": "./dist/types/index.css.d.ts"
    }
  },
}

Finding this was a result of a very frustrating trial an error process, but I'm 100% sure that this is a problem on Next.js' fault. If the original poster could try this I think we could also close the issue :)

EDIT:

After we did some internal investigation, this solution was weirdly only working inside of our monorepo, not in packages consuming our toolkit. The definite absolutely final solution was actually to add our VE-dependant packages to the transpilePackages key on the next.js configuration. Next.js Docs

jneander commented 1 year ago

I appreciate all of the ideas and collaboration here. :heart: I'll make my way back to this eventually. Work has been eating me alive and I don't have anything left in the tank right now for tackling this issue.

uglow commented 1 year ago

Jest config that appears to work in this scenario: NextJS + TypeScript + Jest + VE + module-dependency that includes React components that use VE (library of React+VE components)

jest.config.js

export default async () => {
  const config = await createJestConfig(customJestConfig)();

  // Re-order the VE-jest transform so that it comes first (and it MUST transform BOTH TS **AND JS** files)
  delete config.transform['\\.css\\.(j|t)s$']; // delete existing VE-Jest transform
  config.transform = { '\\.css\\.(j|t)s$': '@vanilla-extract/jest-transform', ...config.transform };

  // vanilla-extract: See https://vanilla-extract.style/documentation/setup/#test-environments
  delete config.moduleNameMapper['^.+\\.(css|sass|scss)$'];

  return config;
};

This works without the VE-babel plugin. The key step is getting the VE-Jest plugin to appear FIRST in the config.transform object. And the VE-jest plugin MUST transform both.css.ts and.css.js files. Hope this helps someone.

smitev commented 1 year ago

After we did some internal investigation, this solution was weirdly only working inside of our monorepo, not in packages consuming our toolkit. The definite absolutely final solution was actually to add our VE-dependant packages to the transpilePackages key on the next.js configuration. Next.js Docs

@otaviomad I can confirm that transpiling the vanilla-extract UI packages solved the issue.

airtonix commented 1 year ago

@jd-oconnor

If you start a thread in discord.

Can we please discourage this habit?

I'd hope that you didn't come to any groundbreaking discoveries in discord, because now it's not searchable from google/bing/duckduckgo/etc.

We can simply hold the conversation here in issue tickets, or the actual discussions tab ☝🏻 resulting in publicly accessible sharing of knowledge.

Even if I did join discord to try an benefit from any outcome there, it'd be near impossible to find that conversation.

jd-oconnor commented 1 year ago

Can we please discourage this habit?

I'd hope that you didn't come to any groundbreaking discoveries in discord, because now it's not searchable from google/bing/duckduckgo/etc.

We can simply hold the conversation here in issue tickets, or the actual discussions tab ☝🏻 resulting in publicly accessible sharing of knowledge.

Even if I did join discord to try an benefit from any outcome there, it'd be near impossible to find that conversation.

Letting people know that there's a community of us that are actively on discord helping people with their issues is not something I will refrain from doing. I understand your concern and it is valid, however, people tend to get help faster when they post in discord. And there is a search feature in the app so you can keyword search for anything you want.

Also, if you scroll up to the top, Adam, also suggested reaching out on discord before I did.

Feel free to join the discord for some quicker feedback from community members if you need some help getting things working.

People ask us for help all the time and often get help right away. It's an incredible resource to be able to get help nearly 24/7 and talk with people about what they need, how they're using VE, and providing more instant feedback. Our help shouldn't be limited to what can be identified by a search engine.

thihathit commented 1 year ago

@viclafouch we had some challenges with VE UI Library to integrate it with Vite. With some tweaking in the Vite config, we succeed to make it work. The hack was to exclude the UI library packages from the "optimizeDeps".

Dep Optimization Options

This is how our vite.config.ts looks now:

import { vanillaExtractPlugin } from "@vanilla-extract/vite-plugin";
import react from "@vitejs/plugin-react-swc";
import { defineConfig } from "vite";

export default defineConfig({
  plugins: [react(), vanillaExtractPlugin()],
  optimizeDeps: {
    exclude: [
      "@filamix-ui",
      "@filamix-ui/generics",
    ],
  },
});

Worth mentioning is that from "@filamix-ui" we are exporting subpaths thru package.json. It works fine. Just make sure in your tsconfig.json to set "compilerOptions" > "moduleResolution" to "Node16".

what a life-saver! this works with Vite. I'm also using subpath exports with my lib(built with VE transpiled to .css.js with tsc).

I'm not sure Vite API supports overwriting optimizeDeps via VE's vite-plugin, but it'd be really nice if vanillaExtractPlugin could support this behavior when package names are explicitly declared like this:

vanillaExtractPlugin({ packages: ['my-library', 'my-friend-library'] })

Update: So it looks like i've encounter another problem. importing .css.js is fine but using normal React components like <Portal /> which uses createPortal from react-dom has a problem with this.

image