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

Replacement is mixing up variables #34

Closed
ericchernuka opened this issue Mar 22, 2022 · 19 comments
Closed

Replacement is mixing up variables #34

ericchernuka opened this issue Mar 22, 2022 · 19 comments

Comments

@ericchernuka
Copy link

ericchernuka commented Mar 22, 2022

I just tried this plugin out, but I'm getting weird behaviour where variables which are numbers and units are being replaced with colours. I've provided an example below. Any thoughts?

image (2)

/* index.scss */
:root {
  --ds-transition-property-base: opacity, transform, background-color,
    box-shadow, border-color, color;
  --ds-transition-duration-base: 200ms;
  --ds-transition-timing-function-base: cubic-bezier(0.4, 0, 0.2, 1);
  --ds-border-radius-base: 0.375rem;
  --ds-spacing-tight: 0.5rem;
  --ds-spacing-base: 1rem;
  --ds-font-size-sm: 0.85rem;
  --ds-font-weight-semi-bold: 600;
  --ds-line-height-sm: 1.25rem;
}
/* Button.module.scss */
.Button {
  transition-property: var(--ds-transition-property-base);
  transition-duration: var(--ds-transition-duration-base);
  transition-timing-function: var(--ds-transition-timing-function-base);
  position: relative;
  outline: none;
  box-sizing: border-box;
  border: 1px solid transparent;
  border-radius: var(--ds-border-radius-base);
  cursor: pointer;
  display: inline-flex;
  align-items: center;
  padding: var(--ds-spacing-tight) var(--ds-spacing-base);
  text-align: center;
  font-size: var(--ds-font-size-sm);
  font-weight: var(--ds-font-weight-semi-bold);
  line-height: var(--ds-line-height-sm);
}
@navanshu
Copy link
Owner

@ericchernuka Can you share the :root as well. It will better help me understand if there is an issue.
Also share the your postcss configuration.

@ericchernuka
Copy link
Author

ericchernuka commented Mar 23, 2022

@navanshu I've updated my description with a more clear delineation between the :root file and the Button SCSS module. I didn't include the full root as it's around 50-75 variables. So just the ones that were shown in the screenshot.

As for the PostCSS config, I'm actually just using Vite's built-in config with SCSS/SASS modules. https://vitejs.dev/config/#css-postcss

So my config looks like the following:

import { defineConfig } from 'vite';
import reactRefresh from '@vitejs/plugin-react-refresh';

// https://vitejs.dev/config/
export default defineConfig({
  // This changes the out put dir from dist to build
  // comment this out if that isn't relevant for your project
  build: {
    outDir: 'build',
  },
  plugins: [reactRefresh()],
  css: {
    postcss: {
      plugins: [require('postcss-variable-compress')]
    }
    
  }
});

@navanshu
Copy link
Owner

navanshu commented Mar 23, 2022

@ericchernuka I am going to need the output root of your code, since I ran the code https://runkit.com/navanshu/ericchernuka-test and it works fine.
As for variable names the code generated work according to the specification. The variable can var names like --0, --11 or --name etc.

Share more code that can be relevant to this.

It would be great if you star my project. It helps me Grow and Provide relevant support

@ericchernuka
Copy link
Author

@navanshu Hmm. that's interesting. I used your runkit and experimented and it looks correct. I'll dig into my pipeline a bit more as something must unexpected must be occurring during the transformation from SCSS modules to the final CSS.

@ericchernuka
Copy link
Author

As I investigate this, I replicated the plugin code so I could log out what it's matching. The additional code I added was the following:

function replacer(match) {
  if (!shouldRun(match)) return match;
  let exist = cssVariablesMap.get(match);
  console.log('match', match, exist ? `exist: ${exist}` : undefined);
  console.log('cssVariablesMap', cssVariablesMap);

  if (!exist) {
    exist = `--${cssVariables.toString(36)}`;
    increase();
    cssVariablesMap.set(match, exist);
    renamedVariables.push(exist);
  }
  return exist;
}

Here is the output from that: https://gist.github.com/ericchernuka/ccadff3b929e477ebd4722a23e822b99

It's almost like the map is reset. I wonder if its a combination of SCSS modules/Vite as the build pipeline, but I haven't come to an insights yet.

@ericchernuka
Copy link
Author

@navanshu I created a reproducible environment where you can see the output I'm seeing. https://stackblitz.com/edit/vitejs-vite-bezk8w?file=src%2FButton.module.scss&terminal=dev

@navanshu
Copy link
Owner

@ericchernuka The map will get reset if postcss is reinitialised or if the files postcss watching is changed, if the order of variables initialisation doesn't change the short names variable get will be same.

The thing that might be getting wrong is You are maybe not importing the scss files correctly

Postcss can be used without sass.

@navanshu
Copy link
Owner

navanshu commented Mar 25, 2022

@ericchernuka Check here https://stackblitz.com/edit/vitejs-vite-adhamz?file=src/Button.tsx. I made some changes to your code.

Unfortunately for now the plugins is not made to be able to replace the variable consistently accross different css files.
The whole css file should be one, all files should be imported before the variables can be replaced.
Check the fork above to understand what I am saying.
You cannot throw it two different files and expect the same result, you need all your css to be one file.
If they are different files you need to merge them.
Maybe what you are want can be achieved but I would need more understanding of how vite or other tooling can be used and how should I make it so the use can be maximised.

@ericchernuka
Copy link
Author

Understood. The only way I can make that happen is by running this as an after-build step then as using SCSS modules avoids me knowing them upfront. I'll have to look at an alternative for now I guess.

Really awesome idea though. In my tests, it reduced our bundle size by 28% just by performing this replacement.

@navanshu
Copy link
Owner

@ericchernuka Thanks I would suggest you write you css in js. Generaly I don't recommend my module as their is GZIP to take of bundle size when the file is one. But I can see when their are multiple files, GZIP will fail. I will put an update to solve this issue.

@navanshu navanshu reopened this Mar 27, 2022
@navanshu
Copy link
Owner

@ericchernuka I have made a seprate project for what you are looking for you can track its progress at https://github.com/Febnik/vitecss-variable-compress

@ericchernuka
Copy link
Author

Amazing! Thank you!

@navanshu
Copy link
Owner

navanshu commented Apr 20, 2022

@ericchernuka Hi
I have updated this library with what you wanted. here is the demo
Feel free to give it a try.
The plugin is handling both the cases

  1. Default Beahviour
  2. The behavious you wanted. For now it is not in index file so you will have to import postcss-variable-compress/splitFiles.js

@navanshu navanshu reopened this Apr 20, 2022
@ericchernuka
Copy link
Author

@navanshu Amazing! I'll try and find some time next week to integrate it. Thanks so much. My prelim tests showed a ~30% reduction in CSS bundle size so I can't wait to utilize this.

@navanshu
Copy link
Owner

@ericchernuka I am happy that it is helpful, to be true I made it, but has not had the time to really use it.
If you could answer one of my questions, how was your experience with vite, do you like the features it gives or it is cumbersome to work with?
I have built my own system, that is similar to vitest that is why I wanted to know.

@ericchernuka
Copy link
Author

I believe vite is the best SPA solution offered today. 0 complaints.

@navanshu
Copy link
Owner

Okay, I kind of dislike being limited by the functionality of a tool I am using.

@ericchernuka
Copy link
Author

I'm a product developer, so if the tooling can do a large amount of the lifting without configuration and I can focus on building great product features, then I'm all for it. Vite is just a breath of fresh air from the speed and ease of configuration if you need to bail out to rollup plugins (which I rarely have had to do).

Worth a shot at least.

@navanshu
Copy link
Owner

Understood.

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

2 participants