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

Rollup & PostCSS generates wrong style files with esm output + preserveModules #818

Closed
FancyVase opened this issue Aug 13, 2021 · 5 comments · Fixed by #822
Closed

Rollup & PostCSS generates wrong style files with esm output + preserveModules #818

FancyVase opened this issue Aug 13, 2021 · 5 comments · Fixed by #822
Labels
bug report 🦗 Issue is probably a bug, but it needs to be checked bundler: rollup 🗞️ Issue is related to rollup bundler needs: complete repro 🖥️ Issue need to have complete repro provided

Comments

@FancyVase
Copy link

Environment

  • linaria: ^3.0.0-beta.4
  • rollup: ^2.5.6
  • node: 13.8.0
  • OS: macOS Mojave 10.14.6

Description

I'm using Rollup with @linaria/rollup + rollup-plugin-postcss to bundle React components that inject their styles on hydration.

I also have an output format of esm w/ preserveModules: true in order to support tree-shaking.

With this configuration, style files get generated incorrectly. Specifically, the built component JS file looks like this:

import React from 'react';
import { styled } from '@linaria/react';
import '../_virtual/LinariaComponent.jsx_52jihk.css';

var Wrapper = /*#__PURE__*/styled("div")({
  name: "Wrapper",
  "class": "w195sr2l"
});
var LinariaComponent = function LinariaComponent() {
  return /*#__PURE__*/React.createElement(Wrapper, null);
};

export { LinariaComponent };
//# sourceMappingURL=LinariaComponent.js.map

I would expect a .js import on the third line instead of import '../_virtual/LinariaComponent.jsx_52jihk.css';

Opening that file gives me:

import styleInject from '../node_modules/style-inject/dist/style-inject.es.js';

var css_248z = ".w195sr2l{background-color:pink;}";
styleInject(css_248z);

export { css_248z as default };
//# sourceMappingURL=LinariaComponent.jsx_52jihk.css.map

Which is valid JS, but not valid CSS, and so attempting to use this component errors out.

This issue only appears when I set the Rollup output.format to 'esm' and set preserveModules: true, which I have to support tree-shaking.


Reproducible Demo

Repo: https://github.com/FancyVase/test-linaria-rollup-postcss

I have a second PostcssComponent.jsx file that gets bundled as expected when importing a .pcss file.

I also included an esm output without preserveModules as well as a cjs output, both of which work as expected.

@FancyVase FancyVase added bug report 🦗 Issue is probably a bug, but it needs to be checked needs: complete repro 🖥️ Issue need to have complete repro provided needs: triage 🏷 Issue needs to be checked and prioritized labels Aug 13, 2021
@github-actions github-actions bot added bundler: rollup 🗞️ Issue is related to rollup bundler and removed needs: triage 🏷 Issue needs to be checked and prioritized labels Aug 13, 2021
@TrevorBurnham
Copy link

TrevorBurnham commented Aug 16, 2021

It looks like the @linaria: prefix in the filename emitted by @linaria/rollup is the culprit here: With that prefix removed, LinariaComponent.jsx_52jihk.css is emitted as a sibling of the JS file (instead of being emitted to _virtual), and it winds up with the expected .js extension after being converted to JS by rollup-plugin-postcss, solving the problem @FancyVase describes.

I'm not familiar enough with Rollup to understand exactly why that prefix might be needed, and I see that it's been there since this plugin was first introduced (#253) [Edit: Whoops! Per Johan's comment below, the prefix was actually added later, in #763.]. @johanholmerin Maybe you can shed some light here for us? 😄

@johanholmerin
Copy link
Contributor

The prefix was added in #763 to support Vite. If that is the issue, maybe @wmzy can help?

@wmzy
Copy link
Contributor

wmzy commented Aug 17, 2021

@FancyVase you can fix the file extension by output.sanitizeFileName config:

import linaria from '@linaria/rollup';
import postcss from 'rollup-plugin-postcss';
import babel from '@rollup/plugin-babel';

export default {
  input: 'src/index.js',

  output: [
    // Works w/ PostCSS, not with Linaria
    {
      dir: 'dist/preserveModules',
      format: 'esm',
      preserveModules: true,
      sourcemap: true,
      sanitizeFileName: (name) => {
        const match = /^[a-z]:/i.exec(name);
        const driveLetter = match ? match[0] : '';
      
        const fileName = driveLetter + name.substr(driveLetter.length).replace(/[\0?*:]/g, '_');
        if (name.startsWith('@linaria:')) {
          return fileName + '.js';
        }
        return fileName;
      },
    },

    // Works w/ PostCSS & Linaria, but doesn't support tree-shaking
    {
      dir: 'dist/noPreserveModules',
      format: 'esm',
      sourcemap: true,
    },

    // Works w/ PostCSS & Linaria, but doesn't support tree-shaking
    {
      file: 'dist/cjs.bundle.js',
      format: 'cjs',
      sourcemap: true,
    },
  ],

  external: ['react'],

  plugins: [
    linaria({
      sourceMap: process.env.NODE_ENV !== 'production',
    }),

    postcss({
      exclude: 'node_modules/**',
    }),

    babel(),
  ],
};

@TrevorBurnham
Copy link

@wmzy Thanks for the workaround! I can confirm that addresses the issue in @FancyVase's demo. 👌

I do still wonder if there might be a better way to address Vite compatibility than using the @linaria: prefix. It looks like the prefix created an earlier issue as well (#778), also with rollup-plugin-postcss. That plugin is very popular, even when not working with PostCSS; there's even a mention in the Linaria BUNDLERS_INTEGRATION docs:

IMPORTANT: rollup-plugin-css-only generates incorrect sourcemaps (see thgh/rollup-plugin-css-only#10). Use an alternative CSS plugin such as rollup-plugin-postcss instead in the same way as above.

@InSuperposition
Copy link

vanilla-extract's Vite plugin takes the following approach to mitigate an open issue in Vite, which might be related.
vitejs/vite#3240.

https://github.com/seek-oss/vanilla-extract/blob/a582934b2696b87e5268a4838350394c7a16a898/packages/vite-plugin/src/index.ts#L22-L35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report 🦗 Issue is probably a bug, but it needs to be checked bundler: rollup 🗞️ Issue is related to rollup bundler needs: complete repro 🖥️ Issue need to have complete repro provided
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants