pascalduez / postcss-apply

PostCSS plugin enabling custom properties sets references
The Unlicense
164 stars 12 forks source link

Back slashes added? #11

Open kevinSuttle opened 7 years ago

kevinSuttle commented 7 years ago

Any ideas on what could be causing this? I'm not sure it's this plugin, to be honest.

screen shot 2016-08-23 at 12 03 22 pm
 postcss: function () {
      return [
        require('postcss-import')({
          addDependencyTo: webpack,
        }),
        customProperties({
          preserve: true,
        }),
        cssApply,
        autoprefixer,
      ]
    },
kevinSuttle commented 7 years ago

I even removed :root and still the back slashes were added.

screen shot 2016-08-23 at 12 30 51 pm
kevinSuttle commented 7 years ago

There seems to be some issue with colons as well. When I don't add them after the "mixins" declared under :root (e.g. --ThemeAnimationStyles), the rules below :root are not eval'd.

pascalduez commented 7 years ago

Hi,

are you sure the @apply plugin is executed actually? Because property sets declarations are supposed to be removed from final output.

Or most probably it's because the slash is added to the declaration before the plugin runs, which breaks its "detection", so nothing happens.

I would advise to always add colons at the end of the declaration. I will add more tests for this.

Can you set-up a small repo to be able to reproduce/debug?

kevinSuttle commented 7 years ago

The plugin is running, because the styles are being applied. Let me see if I can set up a small test repo.

kevinSuttle commented 7 years ago

@pascalduez Here you go: https://cloudup.com/cLsnx9RtW82 See README for repro steps.

kevinSuttle commented 7 years ago

The plugin is running, because the styles are being applied.

Odd. I noticed that they're being applied in Chrome, but not Safari.

screen shot 2016-08-24 at 2 24 54 pm
kevinSuttle commented 7 years ago

Yeah, something's up for sure, because changing the buttons.css file doesn't change anything in Chrome, even though Webpack is "rebuilding".

kevinSuttle commented 7 years ago

Maybe it's this?

https://github.com/postcss/postcss-import/issues/223

pascalduez commented 7 years ago

@kevinSuttle Is this issue still relevant?

pascalduez commented 7 years ago

I tried fiddling with the provided repo for an hour or so, I could not get any progress. Looks like the postcss plugins are not even applied/called. By default storybook will use its own webpack config, so no surprise the plugins aren't used. I added a custom webpack config for storybook but same result.

Next step would be to try without storybook, just "plain" webpack and an index.html.

I'm using cssnext on a projet with webpack n' stuff and have no issue BTW.

kevinSuttle commented 7 years ago

@pascalduez I haven't seen it reproduced. Thanks for checking into it. I appreciate you taking the time.

kevinSuttle commented 7 years ago

Seeing this again now. I'm now using:

Button.css

:root {
  \--ThemeButtonStyles: {
    @apply --ThemeAnimationStyles;
...
}

webpack.config.js

postcss: function() {
      return [
        atImport({
           addDependencyTo: webpack,
           path: './src/Index.css',
           plugins: [
              customProperties({preserve: true}),
              cssApply,
              autoprefixer
          ]
        }),
      ]
    },
pascalduez commented 7 years ago

This is concerning enough, even if Webpack 2 is still in beta. Re-opening.

kevinSuttle commented 7 years ago

Let me elaborate on the structure:

./Index.css

@import "./Themes/Vars.css";
@import "./Themes/Colors/Kratos.css";
@import "./Themes/Colors/Prometheus.css";
@import "./Themes/Mixins.css";

Each component has it's own CSS file. e.g. Button.

Buttons/

Buttons.css is the only component that I am writing a mixin for locally, so maybe that's the repro.

:root {
  --ThemeButtonStyles: {
    @apply --ThemeAnimationStyles; /* From "../Themes/Mixins.css" */
    ...
 }

.btn,
.primaryBtn,
.secondaryBtn,
.pageBtn,
.navBtn {
  @apply --ThemeButtonStyles;

  ... 
  }
}
WARNING in ./~/css-loader!./~/postcss-loader!./src/Index.css
postcss-custom-properties: /Users/kevinSuttle/Code/platform-ui-primitives/src/Themes/Mixins.css:15:5: variable '--PrimaryThemeColor' is undefined and used without a fallback

[repeats]

This is inaccurate. --PrimaryThemeColor is defined in the 2 color files listed before Mixins.css. BUT, something is adding these slashes before they can get there.

kevinSuttle commented 7 years ago

Confirmed. Removing that :root declaration in Button/Buttons.css removed that \, but the warnings still appear. Separate issue it seems.

Or maybe not. I've got another :root declaration in Themes/Mixins.css as well, but I also have them in every top-level file that gets imported into ./Index.css.

:root {
  --ThemeAnimationStyles: {
    transition-duration: .2s;
    transition-timing-function: cubic-bezier(.075, .82, .165, 1);
  };
kevinSuttle commented 7 years ago

Hm. The top-level styles are getting added last, but the component is still getting the correct style applied, which makes this even more confusing.

screen shot 2016-09-20 at 3 50 52 pm
kevinSuttle commented 7 years ago

I added the imports in my top-level npm module export:

require("./Themes/Vars.css");
require("./Themes/Colors/Kratos.css");
require("./Themes/Colors/Prometheus.css");
require("./Themes/Mixins.css");

And got the slashes back on top-level :root declarations. I removed the smart-import plugin and the slashes went away.

I still get the warnings from postcss-custom-properties, but the styles work.

WARNING in ./~/css-loader!./~/postcss-loader!./src/Toggles/Toggles.css
postcss-custom-properties: /Users/kevinSuttle/Code/platform-ui-primitives/src/Toggles/Toggles.css:37:3: variable '--PrimaryThemeColor' is undefined and used without a fallback

Edit: nope. @MoOx, this appears to be an issue with postcss-custom-properties, as it still appears even when I remove postcss-apply.

Will open there.

Memoyr commented 7 years ago

Hi , I got the same issue with the plugin, it was adding backslashes when I try to import custom property sets from an external CSS file. I aim to keep all of my sets in separate file as my project is build with multiple CSS files (file per component). I succeed to import my file and use custom property sets by using postcss-apply in conjonction of postcss-import. There's 2 downside.

I there a better way to achieve this, without those two downside ? Thx

part of my webpack ("webpack": "1.13.3") config : `` var pcssNesting = require('postcss-nesting') var pcssNext = require('postcss-cssnext') var cssApply = require('postcss-apply') var pcssCustomProps = require('postcss-custom-properties') var pcssReporter = require('postcss-reporter') var atImport = require("postcss-import")

postcss: [ pcssCustomProps({preserve: 'computed', strict: false}), pcssNext, atImport(), cssApply, pcssNesting, pcssReporter({clearMessages: true}) ]`

pascalduez commented 7 years ago

@Memoyr Just a quick remark on what you posted above, postcss-custom-properties and postcss-apply are already included within postcss-cssnext. So apparently you're running them twice.
Also the plugins order is important, postcss-import should come first.

pascalduez commented 7 years ago

I have to import the file into my current file anytime I want to use a custom property set.

That's how postcss-import and most preprocessors works.
Only alt solution I can see for now is waiting for #15.

ericsaboia commented 6 years ago

Same is happening here. apply can't do its job because something is adding a \ before the variables. My best guess is that it's css-loader doing it, but I can't understand why.

For now to get unblocked I've created this postcss plugin to clean the extra backslash.

pascalduez commented 6 years ago

@ericsaboia Basically postcss-loader should comes before the css-loader so the apply plugin should already have done it's job. Edit: I don't know what I'm talking about :)

I once tried to debug, but the thing is quite dense, there's so many actors involed... Re-opening anyway.

ericsaboia commented 6 years ago

thanks for looking into this, even though it doesn't seem to be an issue caused by postcss-apply, @pascalduez.

I'm executing postcss-apply as a plugin using extract-text-webpack-plugin + optimize-css-assets-webpack-plugin, so it's only called after all the loaders are executed.

When I execute it inside the loaders it works well, meaning the \ isn't there yet. That's why I believe it might be happening inside css-loader.

I can set up a minimal reproducible environment to help debug this.

yebrahim commented 6 years ago

Also hitting this issue, and also because of css-loader. If I switch to the simple string-loader it works (although imports break), so it seems css-loader does some kind of validation while loading the CSS, and automatically comments invalid CSS out. I can't find a way to turn this off though.

pascalduez commented 6 years ago

Okay, I spotted the culprit. This happens when used together with CSS modules. It's css-selector-tokenizer stringify function.

yebrahim commented 6 years ago

@pascalduez no way around this then?

pascalduez commented 6 years ago

@yebrahim One solution, PR that lib to prevent the escaping. But since custom property sets are not going anywhere, it might be more difficult to get the change accepted, dunno. I'll try, but feel free to.

Currently custom property sets don't have any browser support and will most likely won't get any. So it does not make much sense to use the preserve option, so they basically won't get escaped (problem solved). That might be mitigated by the order the plugin are run though.