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

Persistent cache #667

Open
makenosound opened this issue Dec 10, 2020 · 47 comments
Open

Persistent cache #667

makenosound opened this issue Dec 10, 2020 · 47 comments

Comments

@makenosound
Copy link

Hello! I’ve started playing around with astroturf for a slightly strange project. It’s a library that exports a lot of individual atomic classes using the css API (about 10,000) and so the start-up penalty for astroturf is pretty high. Perfromance is remarkably good once that initial load is complete through.

I’m wondering if it’d be possible to leverage a disk cache alongside the in-memory cache for webpack so that some of that work can be avoid for subsequent startups?

@jquense
Copy link
Contributor

jquense commented Dec 10, 2020

I am hoping that webpack5's persistent cache we give us this but we need to figure out how to make it work with the in memory one. We are using a slightly forked and vendored version of https://github.com/sysgears/webpack-virtual-modules tho which doesn't yet support this (and i've not had the bandwidth to figure it out myself).

@makenosound
Copy link
Author

Thanks for responding, @jquense! If you’re open to it, I could have a look at implementing a persistent cache through that vendored library?

@jquense
Copy link
Contributor

jquense commented Dec 14, 2020

yeah definitely open to it!

@lostfictions
Copy link
Contributor

Would love to have something figured out for this -- I switched to webpack 5 to try to improve long startup times on our project by leveraging the persistent cache, but it seems like it's erroring out on subsequent runs trying to locate the cached versions of the virtual css modules.

Looks like they're trying to figure this out over at sysgears/webpack-virtual-modules#76 but it's not a trivial issue...

@jquense
Copy link
Contributor

jquense commented Feb 27, 2021

Hmm I've not run into actual errors with webpack 5s persistent cache. I know astroturf files aren't in it, but I'd assumed it was just missing the cache and falling back to reprocessing everything. Tho perhaps I've just set it up wrong...

@lostfictions
Copy link
Contributor

It's just as likely it's something in my setup -- I'm using electron-forge which brings its own webpack defaults, so there might be other stuff going on. I could check and see if it repros with a more basic setup.

@jquense
Copy link
Contributor

jquense commented Feb 27, 2021

maybe it's doing some more aggressive caching? I know the older cache-loader definitely didn't work with astroturf on subsequent runs

@lostfictions
Copy link
Contributor

@jquense Turns out it repros for me with a very minimal setup! I've opened a new issue since the fact that it errors out seems distinct from this question of supporting persistent caching: #681

@jquense
Copy link
Contributor

jquense commented Mar 31, 2021

Ok so v1 has a useAltLoader option you can pass in your webpack config for astroturf/loader. This fundamentally changes the approach astroturf takes with webpack, and as a consequence adds persistent cache support. We've been trying it our for a few weeks and it seems to work great, but i'd love more input if you want to give it a whirl

@lostfictions
Copy link
Contributor

Amazing! I'll try to give in a shot next time I spin up a greenfield project or have some time to experiment. Seems like good timing, since Next is starting to roll out production-ready Webpack 5 support.

Seems like Astroturf v1 is getting pretty close to being ready?

@AaronBuxbaum
Copy link

Ok so v1 has a useAltLoader option you can pass in your webpack config for astroturf/loader. This fundamentally changes the approach astroturf takes with webpack, and as a consequence adds persistent cache support. We've been trying it our for a few weeks and it seems to work great, but i'd love more input if you want to give it a whirl

I've found one issue with it -- this functionality:

import { stylesheet } from 'astroturf';
import merge from 'lodash/merge';

const { fontFamily, textColor } = stylesheet`
  :export {
    fontFamily: $font-family-sans-serif;
    textColor: $text-color;
  }
`;

throws the following error, only when the new loader is enabled:

ERROR in ./src/shared/components/plots/Config.tsx? (./src/shared/components/plots/Config-undefined.module.scss!=!./node_modules/astroturf/inline-loader.js?source=/Users/aaron/Work/web/src/shared/components/plots/Config.tsx&styleId=!./src/shared/components/plots/Config.tsx?)
Module build failed (from ./node_modules/mini-css-extract-plugin/dist/loader.js):
ModuleBuildError: Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: expected "{".
  ╷
2 │ import merge from 'lodash/merge';
  │                                 ^
  ╵
  src/shared/components/plots/Config.tsx 2:33  root stylesheet
    at processResult (/Users/aaron/Work/web/node_modules/webpack/lib/NormalModule.js:675:19)
    at /Users/aaron/Work/web/node_modules/webpack/lib/NormalModule.js:777:5
    at /Users/aaron/Work/web/node_modules/loader-runner/lib/LoaderRunner.js:399:11
    at /Users/aaron/Work/web/node_modules/loader-runner/lib/LoaderRunner.js:251:18
    at context.callback (/Users/aaron/Work/web/node_modules/loader-runner/lib/LoaderRunner.js:124:13)
    at /Users/aaron/Work/web/node_modules/sass-loader/dist/index.js:62:7
    at Function.call$2 (/Users/aaron/Work/web/node_modules/sass/sass.dart.js:91729:16)
    at _render_closure.call$0 (/Users/aaron/Work/web/node_modules/sass/sass.dart.js:80354:23)
    at Object.Primitives_applyFunction (/Users/aaron/Work/web/node_modules/sass/sass.dart.js:1086:30)
    at Object.Function_apply (/Users/aaron/Work/web/node_modules/sass/sass.dart.js:6007:16)
 @ ./src/shared/components/plots/Config.tsx 2:0-212 6:4-5
 @ ./src/shared/components/plots/Plotly.tsx 11:0-70 44:40-53 44:56-62 47:14-25 47:35-41
 @ ./src/routes/Dashboard/components/DashboardGraph.tsx
 @ ./src/routes/Dashboard/components/DashboardAnalysisGraph.tsx 5:0-46 10:42-56
 @ ./src/routes/Dashboard/components/DashboardPage.tsx 11:0-62 49:38-60
 @ ./src/routes/Dashboard/index.tsx
 @ ./src/routes.tsx 31:0-48 332:6-20
 @ ./src/configureStore.ts 13:0-30 40:38-44
 @ ./src/createApplication.tsx 10:0-46 17:16-30
 @ ./src/clientApplication.tsx 9:0-52 12:9-26
 @ ./src/client.ts

@jquense
Copy link
Contributor

jquense commented Apr 9, 2021

Aaron is it reproducible with the :export {} syntax?

@AaronBuxbaum
Copy link

Aaron is it reproducible with the :export {} syntax?

I always get the same error, if that's what you're asking. It works if I use the old loader, the only change is that switchover

@jquense
Copy link
Contributor

jquense commented Apr 9, 2021

ah I think this is because the derived identifier for this is "undefined", can you change it to not destructure and see if it fixes it

@AaronBuxbaum
Copy link

ah I think this is because the derived identifier for this is "undefined", can you change it to not destructure and see if it fixes it

This does indeed fix it!

I see one other issue where the new loader creates 1 more CSS file than expected, which causes issues because it changes the way in which some of those affected classes cascade. This may be a combination of the change with some other webpack configuration, or just the loader itself, I'm not sure. Besides that, the system is 💯

@jsg2021
Copy link
Contributor

jsg2021 commented Apr 27, 2021

Ok so v1 has a useAltLoader option you can pass in your webpack config for astroturf/loader. This fundamentally changes the approach astroturf takes with webpack, and as a consequence adds persistent cache support. We've been trying it our for a few weeks and it seems to work great, but i'd love more input if you want to give it a whirl

It was working, but now I'm seeing missing styles... I'll report back with more concrete info after I get my team up and running.

@jsg2021
Copy link
Contributor

jsg2021 commented Apr 27, 2021

@jquense The styles weren't missing, they were being overridden. I'm seeing classes that should be on only one component bleed over to other components defined in the same file. I tried to create a repro in the example dir, but it doesn't seem to work as with the inline loader at all. (class names aren't present in the dom)

@jquense
Copy link
Contributor

jquense commented Apr 28, 2021

@jsg2021 can you post how you've defined styles in that file. I can think of what could produce that but not the situation that would give rise to it. The other helpful detail would be your webpack loader config

@jsg2021
Copy link
Contributor

jsg2021 commented Apr 28, 2021

@jquense This is one example... the file is just a set of parts... the Dot component has the class of Box as well as its own.

export const Container = styled.div`
	cursor: pointer;
	width: 70px;
	height: 70px;
	display: flex;
	align-items: center;
	justify-content: center;

	img,
	svg {
		width: 100%;
	}

	&:global(.flyout-open) {
		background-color: white;
		box-shadow: -1px 0 0 0 #dcdcdc;
		transition: background-color 0.3s, box-shadow 0.3s;
	}
`;

export const Box = styled.div`
	position: relative;
	width: 42px;
	height: 42px;
`;

export const Dot = styled(User.Presence).attrs({ me: true })`
	position: absolute;
	right: 2px;
	bottom: 2px;
`;

the js loader:

{
			test: jsTestExp,
			use: [
				{
					loader: 'babel-loader',
					options: {
					...
					},
				},
				{
					loader: 'astroturf/loader',
					options: {
						allowGlobal: true,
						useAltLoader: true,
					},
				},
			]
		},

css loader:

{
	test: /\.css$/,
	sideEffects: true,
	use: [
		'style-loader',
		{
		loader: 'css-loader',
		options: {
			sourceMap: true,
			importLoaders: 1,
			modules: {
				exportGlobals: true,
				exportLocalsConvention: 'camelCase',
				localIdentName: '[local]--[hash:base64:8]',
			},
		}},
		//postCss
	],
}

@jquense
Copy link
Contributor

jquense commented Apr 28, 2021

ok the code looks good, you might need to upgrade css-loader, make sure you have the latest version

@jsg2021
Copy link
Contributor

jsg2021 commented Apr 28, 2021

@jquense I do. works until altLoader is used. This code may not reproduce in isolation. It may require a large project.

1 similar comment
@jsg2021
Copy link
Contributor

jsg2021 commented Apr 28, 2021

@jquense I do. works until altLoader is used. This code may not reproduce in isolation. It may require a large project.

@jquense
Copy link
Contributor

jquense commented Apr 28, 2021

hmm we have this one in a a few fairly large projects and aren't seeing it, so maybe it's not specifically size related, but something with added complexity. if you pull just this file out a with the same webpack config do you get the same issue?

@jsg2021
Copy link
Contributor

jsg2021 commented Apr 28, 2021

Ohhh... I think the class names are colliding.
image

the .cls2--3LXraOKI in this pick is showing styles from three unique components

@jsg2021
Copy link
Contributor

jsg2021 commented Apr 28, 2021

@jquense each of those .cls2--3LXraOKI are from unique components from the same file.

@jquense
Copy link
Contributor

jquense commented Apr 28, 2021

are you sure you have the latest css-loader? I would expect this to happen on an slightly older release, basically the hash seems like it's not taking into account part of the resource name, which is producing conflicts

@jsg2021
Copy link
Contributor

jsg2021 commented Apr 28, 2021

@jquense I'm on css-loader 5.2.4.

@jquense
Copy link
Contributor

jquense commented Apr 28, 2021

arg very confusing. if it's possible to put a repro together i can inspect that would help a ton

@jsg2021
Copy link
Contributor

jsg2021 commented Apr 28, 2021

Got lucky... reproduces in this: https://github.com/jsg2021/astroturf-classname-collsion-repro

@jsg2021
Copy link
Contributor

jsg2021 commented Apr 28, 2021

@jquense if you comment out useAltLoader that repo produces classes I as I would expect... with it on, however, the classes become overlapped.

@jquense
Copy link
Contributor

jquense commented Apr 28, 2021

ok it seems like i had already fixed it and released it as 1.0.0-beta.21 but for some reason i cannot find that tag. in any case upgrading seemed to fix it

@lostfictions
Copy link
Contributor

lostfictions commented May 24, 2021

This classname collision bug just happened to me with useAltLoader in 1.0.0-beta.21, unfortunately...

I'd written something like this:

const touchNone = css`
  touch-action: none;
`;

then was experimenting, and added another declaration above it:

const touchPan = css`
  touch-action: pan-y;
`;

const touchNone = css`
  touch-action: none;
`;

I was using touchNone in an element and swapped it for touchPan, but it wasn't working -- inspecting the element showed that both classes were being applied, and touchNone was winning out.

Is there a chance that it's getting confused by declaration order? I think the only thing I did differently than normal here is declaring something above an existing declaration.

@jquense
Copy link
Contributor

jquense commented May 25, 2021

order shouldn't matter, it's mostly due to what css-loader hashes. What is your css-loader options and version?

@lostfictions
Copy link
Contributor

lostfictions commented May 25, 2021

It's Next's built-in CSS handling, which I think uses their own internal fork of css-loader. (yarn why css-loader only shows the version bundled with astroturf.) I append astroturf to Next's webpack rules in next.config.js like this:

const withPlugins = require("next-compose-plugins");

module.exports = withPlugins(
  [
    () => ({
      webpack(cfg) {
        cfg.module.rules.push({
          test: /\.tsx$/,
          use: [{ loader: "astroturf/loader", options: { useAltLoader: true } }],
        });

        // unrelated loader config elided...

        return cfg;
      },
    }),
    // unrelated next plugins elided...
  ],
  {
    future: {
      webpack5: true,
    },
  }
);

...which has always worked for me. This is with [email protected].

I'll see if I can make a simpler repro at some point. I'm not sure, but I think stopping and restarting Next's dev server actually fixed the issue. So it might be transient, as a result of editing, rather than a persistent failure under a specific config?

If it helps, here are the relevant bits from the two configs (server and client) that Next spits out when I append
console.dir(cfg.module, { depth: null }) just before return cfg; in the above snippet.

Server:

{
  rules: [
    // ...
    {
      test: /\.(tsx|ts|js|mjs|jsx)$/,
      include: [
        '/<project root>',
        /next[\\/]dist[\\/]next-server[\\/]lib/,
        /next[\\/]dist[\\/]client/,
        /next[\\/]dist[\\/]pages/,
        /[\\/](strip-ansi|ansi-regex)[\\/]/
      ],
      exclude: [Function: exclude],
      use: {
        loader: 'next-babel-loader',
        options: {
          configFile: undefined,
          isServer: true,
          distDir: '/<project root>/.next',
          pagesDir: '/<project root>/src/pages',
          cwd: '/<project root>',
          cache: false,
          development: true,
          hasReactRefresh: false,
          hasJsxRuntime: true
        }
      }
    },
    // ...
    {
      oneOf: [
        // ...
        {
          sideEffects: false,
          test: /\.module\.css$/,
          issuer: {
            and: [ '/<project root>' ],
            not: [ /node_modules/ ]
          },
          use: [
            {
              loader: '/<project root>/node_modules/next/dist/compiled/css-loader/cjs.js',
              options: {
                importLoaders: 1,
                sourceMap: true,
                esModule: false,
                url: [Function: cssFileResolve],
                import: [Function: import],
                modules: {
                  exportLocalsConvention: 'asIs',
                  exportOnlyLocals: true,
                  mode: 'pure',
                  getLocalIdent: [Function: getCssModuleLocalIdent]
                }
              }
            },
            {
              loader: '/<project root>/node_modules/next/dist/compiled/postcss-loader/cjs.js',
              options: {
                postcssOptions: {
                  plugins: [
                    [Function (anonymous)] { postcss: true },
                    [Function (anonymous)] {
                      postcssPlugin: 'postcss-preset-env',
                      postcssVersion: '7.0.32'
                    }
                  ],
                  config: false
                },
                sourceMap: true
              }
            }
          ]
        },
        // ...
      ]
    },
    // ...
    { test: /\.tsx$/, use: [ { loader: 'astroturf/loader', options: { useAltLoader: true } } ] }
  ]
}

Client:

{
  rules: [
    // ...
    {
      test: /\.(tsx|ts|js|mjs|jsx)$/,
      include: [
        '/<project root>',
        /next[\\/]dist[\\/]next-server[\\/]lib/,
        /next[\\/]dist[\\/]client/,
        /next[\\/]dist[\\/]pages/,
        /[\\/](strip-ansi|ansi-regex)[\\/]/
      ],
      exclude: [Function: exclude],
      use: [
        '/<project root>/node_modules/@next/react-refresh-utils/loader.js',
        {
          loader: 'next-babel-loader',
          options: {
            configFile: undefined,
            isServer: false,
            distDir: '/<project root>/.next',
            pagesDir: '/<project root>/src/pages',
            cwd: '/<project root>',
            cache: false,
            development: true,
            hasReactRefresh: true,
            hasJsxRuntime: true
          }
        }
      ]
    },
    {
      oneOf: [
        // ...
        {
          sideEffects: false,
          test: /\.module\.css$/,
          issuer: {
            and: [ '/<project root>' ],
            not: [ /node_modules/ ]
          },
          use: [
            {
              loader: 'next-style-loader',
              options: { insert: [Function: insert] }
            },
            {
              loader: '/<project root>/node_modules/next/dist/compiled/css-loader/cjs.js',
              options: {
                importLoaders: 1,
                sourceMap: true,
                esModule: false,
                url: [Function: cssFileResolve],
                import: [Function: import],
                modules: {
                  exportLocalsConvention: 'asIs',
                  exportOnlyLocals: false,
                  mode: 'pure',
                  getLocalIdent: [Function: getCssModuleLocalIdent]
                }
              }
            },
            {
              loader: '/<project root>/node_modules/next/dist/compiled/postcss-loader/cjs.js',
              options: {
                postcssOptions: {
                  plugins: [
                    [Function (anonymous)] { postcss: true },
                    [Function (anonymous)] {
                      postcssPlugin: 'postcss-preset-env',
                      postcssVersion: '7.0.32'
                    }
                  ],
                  config: false
                },
                sourceMap: true
              }
            }
          ]
        },
        // ...
      ]
    },
    // ...
    { test: /\.tsx$/, use: [ { loader: 'astroturf/loader', options: { useAltLoader: true } } ] }
  ],
}

@jquense
Copy link
Contributor

jquense commented May 25, 2021

next is probably using an old css-loader that doesn't support this :/

@lostfictions
Copy link
Contributor

lostfictions commented May 25, 2021

Maybe! I'm not sure. One of the big-ticket items of Next 10.1/10.2 was Webpack 5 support, specifically highlighting persistent caching. And useAltLoader didn't error out -- like I said, it seemed to work fine for quite a while until the collision bug (which seemed identical to the one already raised in this thread) occurred.

But I think it should be possible to override Next's built-in CSS handling and use stock css-loader instead, so maybe I can confirm that. I'll try to see if I can come up with a repro at some point!

@jquense
Copy link
Contributor

jquense commented May 25, 2021

A repro would be awesome, that's the easiest way to troubleshoot!

@lostfictions
Copy link
Contributor

I've provided a repro in #706!

@lostfictions
Copy link
Contributor

Hey @jquense, just thought I'd give another small poke on this -- have you had a chance to look at the repro? The issue isn't Next-specific -- I've provided a minimal repro here demonstrating the issue, as mentioned in #706 (comment). Sounds like I'm not the only one still having this problem, as #705 suggests... Unfortunately it's kind of a dealbreaker on one project at this point, so if it's not resolvable we're looking at migrating to something else.

@jquense
Copy link
Contributor

jquense commented Jul 26, 2021

hey, no i've not had a chance to look over this, may be able to look this week

@jquense
Copy link
Contributor

jquense commented Jul 26, 2021

@lostfictions I took a very quick look of the latest repro. The reason it's broken is because the localIdentName does not contain a hash. Change it to "[path][name]__[local]_[hash:5]" and the classes do not conflict

@lostfictions
Copy link
Contributor

@jquense This makes a lot of sense, thanks! And I think it explains why things are broken in Next with the alternate loader, too -- seems like they use a custom getLocalIdent with a hash that doesn't take the context into account.

Looking around a bit more, it seems like some folks worked out a solution for using Linaria and Next together while leaning on Next's built-in CSS support. It configures Linaria to export under a .linaria.module.css extension and then rewrites Next's existing rules to check for that extension and use it to decide whether to defer to Next's getLocalIdent or not. Kind of a dirty hack, but I guess something similar is probably necessary to get Next working with Astroturf again.

@jquense
Copy link
Contributor

jquense commented Jul 26, 2021

I don't own the next-astroturf stuff but would love to see that happen. Anyone want to work on it?

@lostfictions
Copy link
Contributor

lostfictions commented Jul 26, 2021

Okay, based on the above I threw together a fix that seems to solve my issues -- it defers to Next's local identifier generation but appends the resource query (ie. the identifier the css tag is assigned to, which is the missing piece of context for correct classname generation) if it finds Astroturf's alt loader in the Webpack request.

I haven't tested this in other scenarios and don't know enough about Webpack to tell if my technique plays well with anything else or is brittle -- so I don't feel too confident offering this as a complete fix without some feedback and further testing. But it could potentially be the start of a drop-in Next plugin to enable Astroturf?

(In the interim, unfortunately it looks like Next's official with-astroturf example was removed a month back due to lack of maintenance -- probably not helped by the perceived Webpack 5 incompatibility, since they rolled the latter out to all users by default soon after.)

@dreyks
Copy link
Contributor

dreyks commented Oct 4, 2021

I'm getting has collisions when using alt loader

this happens when there are several styled components in a single js file. alt loader attaches the component name as a query path/to/file.js?ComponentName, but css-loader only considers the resourcePath part (which does not contain the query). Since the cls1/cls2 class names are same for all the components this leads to same hashes
https://github.com/webpack-contrib/css-loader/blob/b29d3899f8130e77e1ad6cf4c4c2fe18116abcd1/src/utils.js#L323

I'm not really sure how to proceed with this since css-loader doesn't pass the full resource to webpack and thus webpack's native [query] placeholder doesn't work either

@jquense
Copy link
Contributor

jquense commented Oct 4, 2021

you need to add a [hash] to your local ident convention config in css-loader, it considers the match resource which is set

@dreyks
Copy link
Contributor

dreyks commented Oct 4, 2021

yeah I had [hash] but I understand now that I had other issues - match resource being absent due to #727

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants