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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰CSS Importing effectively broken #1165

Open
jssee opened this Issue Apr 10, 2018 · 38 comments

Comments

Projects
None yet
@jssee
Copy link

jssee commented Apr 10, 2018

The way Parcel's CSS import is currently being implemented broken. There are several issues related to this problem. Basically, Parcel is applying postcss transforms before concatenating the CSS, this means that any CSS that gets imported is not being transformed properly. This breaks functionality and won't allow you to use basic features like css-variables if they are being imported anywhere.

related to this:
#609
#593
#329

馃 Expected Behavior

I should be able to declare variables in one file and use them in another via @import without getting undefined errors

馃槸 Current Behavior

Parcel is not transforming variables that are in imported css files.

馃拋 Possible Solution

Concatenate the CSS files (i.e. perform import) before applying transforms. Or let users use the postcss-import plugin from their own config to get things working properly.

@irritant came up with a patch that was as simply commenting out the import gathering during the collectDependencies() process, this should perhaps be the default until something is figured out.

馃捇 Code Sample

other.css

:root {
  --varColor: #639;
}

input.css

@import './other.css';

body {
  color: var(--varColor);
}

output.css

body {
  color: var(--varColor);  /* should be #639 */
}

some more examples can also be found in #329

@DeMoorJasper

This comment has been minimized.

Copy link
Member

DeMoorJasper commented Apr 10, 2018

What about gathering variables using postcss and applying them down the tree? Gathering in generate and applying in postGenerate...
This way parcel won't lose it's parallel behaviour, yet support this?

@jssee

This comment has been minimized.

Copy link
Author

jssee commented Apr 10, 2018

@DeMoorJasper I'm not sure, another thing i thought about was doing running collectDependencies() inside preTransforms() but i have no idea if that would work.

@irritant

This comment has been minimized.

Copy link

irritant commented Apr 10, 2018

@jssee

This comment has been minimized.

Copy link
Author

jssee commented Apr 10, 2018

@irritant I admire your commitment to Parcel but I think at that point it might be easier to just use Gulp as the build tool or even plain Webpack. I have to make that decision myself since unfortunately I cant stop work waiting for this issue gets fixed, Parcel was really great for everything else though.

@DeMoorJasper

This comment has been minimized.

Copy link
Member

DeMoorJasper commented Apr 10, 2018

If parcel isn't responsible for collecting and parsing the dependencies it doesn't know about it resulting in the manual force recompiling.
Not sure what even makes this work, are you using sass?

@michael-ciniawsky

This comment has been minimized.

Copy link

michael-ciniawsky commented Apr 10, 2018

.postcssrc.js

const variables = {
  '--red': '#12345'
}

module.exports = {
  plugins: [
     require('postcss-custom-properties')({
         variables // <= Available to all modules (@imports)
     })
  ]
}

鈿狅笍 Don't use a postcss-import-* plugin as parcel collects the dependencies (@imports)

@jssee

This comment has been minimized.

Copy link
Author

jssee commented Apr 10, 2018

@DeMoorJasper No sass, just a simple postcss.confg.js which is only running postcss-preset-env to "polyfill" the CSS. I've tried switching to postcss-cssnext to do the same thing but the issue persist, and the same thing will happen if you try to use PreCSS and or even postcss-simple-vars

@devongovett

This comment has been minimized.

Copy link
Member

devongovett commented Apr 10, 2018

Parcel processes each CSS file independently, which is why your imported variables aren't being replaced. This strategy makes it faster since we can process each CSS file in parallel rather than one huge concatenated at the end.

Seems like CSS always allowed each file to be processed independently until variables were added. I can't think of another case where this is a problem. Perhaps we could detect if postcss variables are being used and enable a post-processing step in the CSSPackager to replace variables. Other postcss plugins would still be applied to each individual file. This would require re-parsing the concatenated css though, which would be sub-optimal.

@devongovett

This comment has been minimized.

Copy link
Member

devongovett commented Apr 10, 2018

Actually that probably wouldn't work since other postcss plugins may rely on the variable already being replaced earlier. e.g. a plugin that inlines calc() calls might expect calc(var(--my-var) + 2) to have the variable inlined first so it could do the calculation ahead of time.

We definitely don't want to do all postcss processing on the final concatenated source though. hmm...

@jssee

This comment has been minimized.

Copy link
Author

jssee commented Apr 10, 2018

@devongovett What is the reason we dont want postcss processing on the final concatenated file? Is that not how every other tool does this type of stuff?

@irritant

This comment has been minimized.

Copy link

irritant commented Apr 10, 2018

@jssee For me, the minor inconvenience of maintaining my fork and running a separate gulp task is worth Parcel's other benefits.

My gulp task is:

gulp.task('invalidate-css-index', function() {
  gulp.src('./src/css/index.css').pipe(gulp.dest('./src/css'));
});

gulp.task('watch-css', function() {
  gulp.watch(['./src/css/**/*.css', '!./src/css/index.css'], ['invalidate-css-index']);
});

Piping index.css to itself (basically equivalent to saving the file) is enough to make Parcel recognize that the file has been touched and recompile the CSS. With collectDependencies commented out, postcss-import handles the imports and allows later postcss plugins to work properly.

@jssee

This comment has been minimized.

Copy link
Author

jssee commented May 11, 2018

@devongovett any progress on this?

@twoixter

This comment has been minimized.

Copy link

twoixter commented May 20, 2018

I recently discovered Parcel and used it from last couple days (it's great!) and I'm also banging my head around CSS issues. Here's my 2 cents, specially after reading @devongovett comment:

Parcel processes each CSS file independently, which is why your imported variables aren't being replaced. This strategy makes it faster since we can process each CSS file in parallel rather than one huge concatenated at the end.

Although clever, and desirable, this is not how CSS is generally processed. CSS is not JS and although PostCSS could come close (I don't know enough about PostCSS), certainly LESS is not.

AFAIK CSS @import does not work like ES6 import. At least, not semantically. This is illustrated by the following LESS snippet:

@background-color: red;

body {
    background: @background-color;
}

@background-color: blue;

What would the processed body { background: ??? } be in this snippet? Well, the color is blue processed with LESS and red processed with SASS. (OK, SASS uses $ for variables, bear with me).

In other words, although the snippet does not have any @import I think you get the point: variables in LESS are "lazy" processed, so they get the LAST assigned value even if it is assigned at the very end of the last .less file. SASS on the other hand replaces variables with the value they had at that moment while processing.

Both LESS and SASS require processing the whole thing anyway. LESS for obvious reasons: it requires a 2-pass compilation. But as a general rule of thumb CSS processors never had the notion of "scopes" so importing a CSS file does not open a new "scope" or whatever. It is expected for a .less / .sass file to get access to the "global" namespace, so to speak, so you don't have to @import "variables" in every single file.

@mendrik

This comment has been minimized.

Copy link

mendrik commented May 24, 2018

not sure if it helps anyone but I have published a small postcss plugin as a temporary work around for my project: npm install postcss-parcel-import
then add it to your .postcssrc or config:

{
    "plugins": {
        "postcss-import": {},
        "postcss-parcel-import": {},
        "postcss-mixins": {},
        "postcss-nested": {},
        "autoprefixer": {},
        "cssnano": {},
    }
}

and in the pcss files use then @parcel-import '.../../mixins.pcss'

basically it hides it from parcel importing and will substitute the rule with the file contents before other plugins are run.

This is just a temporary solution until this bug is solved.

Source code under MIT: https://github.com/mendrik/postcss-parcel-import

@cmnstmntmn

This comment has been minimized.

Copy link

cmnstmntmn commented Aug 27, 2018

my solution was to build in two steps.

  "scripts": {
    "build": "yarn run parcel && yarn run postcss",
    "postcss": "postcss -c scripts/postcss.config.js --replace src/css/app.css -o dist/*.css",
    "parcel": "parcel build index.html"
  }

and run yarn run build

@tatsujb

This comment has been minimized.

Copy link

tatsujb commented Oct 29, 2018

I'm not sure if this is a stupid question or not but if I'm using scss only and in one file (root.scss), I do :

    $grey: #cacaca;

and then in another file (app.scss) I do :

    @import 'root';

    .my-class{
           background: $grey;
     }

...will this trigger the same undefined error under parcel ? (thinking of switching to parcel)

@andreidcm

This comment has been minimized.

Copy link

andreidcm commented Nov 20, 2018

Going on what @michael-ciniawsky said, this works:

variables.css

:root {
  --distancer: 30px;
}

button.css

.button {
  padding: var(--distancer);
}

.postcssrc

{
  "modules": true,
  "plugins": {
    "postcss-custom-properties": {
      "preserve": false,
      "importFrom": "src/styles/variables.css"
    }
  }
}
@dreerr

This comment has been minimized.

Copy link

dreerr commented Dec 27, 2018

Having similar issues with postcss-extend, when referencing from another file:

base.pcss
.class {
color: red;
}

index.pcss
@import 鈥渂ase.pcss鈥
body {
@extend .class;
}

The example above does not render an error nor populate the body with red color. The idea of rendering every file individually does not make sense to me, as this should all be possible with postcss
+1 for being able to use a native postcss-import!

@aoberoi

This comment has been minimized.

Copy link

aoberoi commented Jan 14, 2019

i'm also facing a similar issue. i'd like to use postcss-mixins, where a @define-mixin happens in foo.css, and the @mixin happens in bar.css (where @import './foo.css'; appears earlier). i cannot get this to work with parcel - i get an Undefined mixin error.

@mischnic

This comment has been minimized.

Copy link
Member

mischnic commented Jan 14, 2019

The example code in the issue description works correctly with the current Parcel:

:root {
  --varColor: #639;
}body {
  background-color: var(--varColor);
}
@cmnstmntmn

This comment has been minimized.

Copy link

cmnstmntmn commented Jan 14, 2019

@mischnic the problem is with imports;

if you split your code example into:

config.css

:root {
  --varColor: #639;
}

and app.css

@import "./config.css"

body {
 background-color: var(--varColor);
}

variables won't be substituted.

@dreerr

This comment has been minimized.

Copy link

dreerr commented Jan 14, 2019

Imports should be handled by postcss directly and not by Parcel!!

@mischnic The problem is that the postcss-import module is not executed in the way it should be, as imports are handled by Parcel, but only after the postcss transformation has been made!

See: #609 (comment)

My current workaround is also @cmnstmntmn #1165 (comment)

This is the same issue as CSS files being compiled with PostCSS before they're imported #609

@mischnic

This comment has been minimized.

Copy link
Member

mischnic commented Jan 14, 2019

@cmnstmntmn @dreerr

the problem is with imports;
if you split your code example into:

Please take a look at this (from #1165 (comment)), it works: (or do you have some postcss configuration)
https://github.com/mischnic/css-import-var

Imports should be handled by postcss directly and not by Parcel!!
The problem is that the postcss-import module is not executed in the way it should be, as imports are handled by Parcel, but only after the postcss transformation has been made!

Yes, I think disabling Parcel's import handling if postcss-import is used would work (and make it use parcels resolver)?

@dreerr

This comment has been minimized.

Copy link

dreerr commented Jan 14, 2019

@mischnic the example at https://github.com/mischnic/css-import-var is missing a postcss config with postcss-custom-properties and should correctly output:

:root{
  --varColor: #639;
}
body{ 
  background-color: #639;
  background-color: var(--varColor);
}

(See https://github.com/postcss/postcss-custom-properties)

IMHO the Parcel import should be replaced by the postcss-import or be executed after the postcss files have been generated.

@mischnic

This comment has been minimized.

Copy link
Member

mischnic commented Jan 14, 2019

IMHO the Parcel import should be replaced by the postcss-import or be executed after the postcss files have been generated.

But postcss doesn't handle SASS or LESS imports.

@DeMoorJasper

This comment has been minimized.

Copy link
Member

DeMoorJasper commented Jan 14, 2019

@mischnic sass and less already uses it's own importer (so changing to postcss-import wouldn't change this at all a far as I know)

@mischnic

This comment has been minimized.

Copy link
Member

mischnic commented Jan 14, 2019

sass and less already uses it's own importer (so changing to postcss-import wouldn't change this at all a far as I know)

I was referring to a css file importing sass/less (though that could be done with some configuration).

I think we should only do this if there is some postcss configuration, to retain performance for "vanilla" CSS.

@sj26

This comment has been minimized.

Copy link
Contributor

sj26 commented Jan 20, 2019

I'm hitting this issue extensively trying to build a static html page using basscss. But any project which has a number of css files, some defining variables and custom media queries, and others consuming those definitions, will have this issue. It seems like maybe there need to be two postcss passes 鈥斅爋ne over the individual asset, and one over the packaged bundle.

Would parcel consider adding a bundle transform phase?

Or perhaps the individual assets could be parsed and partially transformed in parallel, then the asts could be merged by the packager and additional transforms run? This might require differentiating asset plugins from bundle plugins somehow.

@devongovett

This comment has been minimized.

Copy link
Member

devongovett commented Jan 21, 2019

I think we could maybe solve this issue by moving postcss to the pretransform phase, which occurs prior to dependencies being parsed. Currently, it is applied in the transform phase, which happens after @imports are already parsed.

async transform() {
await postcssTransform(this);
}

If that method is simply renamed to pretransform rather than transform postcss will be applied first, meaning that if a user specifies postcss-import in their .postcssrc, then imports will be compiled away by postcss instead of being handled by parcel. If not, or there is no .postcssrc, then parcel will handle the imports as it does today.

Would someone like to prototype this and create a PR?

@sj26

This comment has been minimized.

Copy link
Contributor

sj26 commented Jan 22, 2019

I published a little parcel plugin which swaps the CSSAsset transform function to be pretransform and it fixes the issue in the website I'm working on, in combination with postcss-imports and friends:

https://github.com/sj26/parcel-plugin-css-pretransform

So perhaps renaming will just work?

@devongovett

This comment has been minimized.

Copy link
Member

devongovett commented Jan 22, 2019

@sj26 would you like to send a PR?

@mischnic

This comment has been minimized.

Copy link
Member

mischnic commented Jan 22, 2019

Should postcss-import use Parcel's resolver?

@devongovett

This comment has been minimized.

Copy link
Member

devongovett commented Jan 22, 2019

@mischnic ideally, but I feel like if users are specifying to use postcss-import instead of the default parcel behavior then that is their problem not ours, so if we don't support using parcel's resolver with postcss-import then that's probably ok.

@cmnstmntmn

This comment has been minimized.

Copy link

cmnstmntmn commented Jan 24, 2019

hmm, does anyone manage to make it work, using @sj26 's
parcel-plugin-css-pretransform?

this is my .postcssrc file. the result is as without the plugin

{
    "modules": false,
    "plugins": {
      "postcss-import": false,
      "postcss-custom-media": true, 
      "postcss-custom-properties": true,
      "autoprefixer": {
        "browsers": ["last 2 versions"],
        "grid": true
      }
    }
}
@mischnic

This comment has been minimized.

Copy link
Member

mischnic commented Jan 24, 2019

@cmnstmntmn Should be "postcss-import": true

@cmnstmntmn

This comment has been minimized.

Copy link

cmnstmntmn commented Jan 24, 2019

hmm, this seems to be the problem

parcelServer running at http://localhost:1234
鈿狅笍  Cannot find module 'parcel-bundler/src/assets/CSSAsset'
    at Function.Module._resolveFilename (module.js:547:15)
    at Function.Module._load (module.js:474:25)
    at Module.require (module.js:596:17)
    at require (/Users/constantin/.config/yarn/global/node_modules/v8-compile-cache/v8-compile-cache.js:159:20)
    at module.exports (/Users/constantin/apps/cora-ui/node_modules/parcel-plugin-css-pretransform/index.js:2:19)
    at Bundler.loadPlugins (/Users/constantin/.config/yarn/global/node_modules/parcel-bundler/src/Bundler.js:209:17)
    at <anonymous>
@lostboy

This comment has been minimized.

Copy link

lostboy commented Feb 16, 2019

@cmnstmntmn you can get past that by adding parcel-bundler to your project and running parcel from an npm script or with npx

@mischnic

This comment has been minimized.

Copy link
Member

mischnic commented Feb 26, 2019

One issue is that, with this change, Parcel doesn't know anything about the dependencies of a CSS file, and so HMR (and caching) for these doesn't work anymore.
We would first need to recursively search for imports and add these as a dependency before postcss runs (so parsing every CSS file twice).

@jhpratt jhpratt referenced this issue Mar 19, 2019

Open

Move to Parcel #3

0 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.