Skip to content

css warning "variable '--some-var' is undefined" #8289

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

Closed
amitport opened this issue Nov 2, 2017 · 21 comments · Fixed by #8874
Closed

css warning "variable '--some-var' is undefined" #8289

amitport opened this issue Nov 2, 2017 · 21 comments · Fixed by #8874
Assignees
Labels
help wanted P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful type: bug/fix

Comments

@amitport
Copy link

amitport commented Nov 2, 2017

Bug Report or Feature Request (mark with an x)

- [X] bug report -> please search issues before submitting
- [ ] feature request

Versions.

Angular CLI: 1.5.0
Node: 8.9.0
OS: win32 x64
Angular: 5.0.0
... animations, common, compiler, compiler-cli, core, forms
... http, language-service, platform-browser
... platform-browser-dynamic, router

@angular/cdk: 2.0.0-beta.12
@angular/cli: 1.5.0
@angular/flex-layout: 2.0.0-beta.10
@angular/material: 2.0.0-beta.12
@angular-devkit/build-optimizer: 0.0.32
@angular-devkit/core: 0.0.20
@angular-devkit/schematics: 0.0.35
@ngtools/json-schema: 1.1.0
@ngtools/webpack: 1.8.0
@schematics/angular: 0.1.0
typescript: 2.4.2
webpack: 3.8.1

Repro steps.

add to global styles.scss:

:root {
  --some-var: blue;
}

add to some component scss:

.some-class {
  background: var(--some-var);
}

ng serve

The log given by the failure.

NonErrorEmittedError: (Emitted value instead of an instance of Error) postcss-custom-properties:
<my-component>.scss:2:3: variable '--some-var' is undefined and used without a fallback
    at Object.emitWarning (<path>\node_modules\webpack\lib\NormalModule.js:117:16)
    at <path>\node_modules\postcss-loader\index.js:131:24
    at Array.forEach (<anonymous>)
    at <path>\node_modules\postcss-loader\index.js:130:31
    at <anonymous>

Desired functionality.

no warning needed, the component is defined in the global styles.scss and everything works

Mention any other details that might be useful.

one workaround is the create a _variables.scss with all the global :root { --my-var ... } and include this files in components. This will make to warning go away but the declarations in _variables.scss will be included few times

@filipesilva
Copy link
Contributor

Thanks for reporting this issue. However, this issue is a duplicate of an existing issue #1253. Please subscribe to that issue for future updates.

@amitport
Copy link
Author

amitport commented Nov 2, 2017

@filipesilva this issue is about native css variables. Including a variables file make sense for #1253 and scss, but not in this case. Here, the global css code will be added to the bundle for each include.

(#1253 is more about the need to specify a relative path, scss variable declaration is removed in compilation)

Can you please take a second look, Thank you!

@filipesilva
Copy link
Contributor

Thank you for making that clear, I saw the scss files and thought it was the same thing. I'll reopen.

I don't have an answer for it right now though...

@filipesilva filipesilva reopened this Nov 2, 2017
@filipesilva filipesilva self-assigned this Nov 2, 2017
@filipesilva filipesilva added command: build help wanted P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent type: bug/fix labels Nov 2, 2017
@clydin
Copy link
Member

clydin commented Nov 2, 2017

Currently postcss-custom-properties is unconditionally enabled even in the case where the app will only be used on browsers that natively support css variables. Among postcss-custom-properties limitations is that it needs full knowledge of the context. In this case that cannot be known at build time hence the warning.

The most appropriate solution would be to only enable the plugin when the app needs to support browsers that do not have native css variable support.

@filipesilva filipesilva added P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful and removed P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Nov 3, 2017
@SanderElias
Copy link

@clydin I ran into the same issue, and your explanation makes sense. My project only uses default CSS, and is targeting supported browsers only, so your solution would be ideal in my situation too.

Although I only get the issue in cli 1.5 as I compile without AOT. If AOT is enabled, the warning is gone. Not sure why that happens.

@mmc41
Copy link

mmc41 commented Nov 4, 2017

Also, using native css only (no scss) and got the same problem. Eagerly awaiting a fix for this.

@cedvdb
Copy link

cedvdb commented Nov 7, 2017

@clydin This feature is really a nice to have (I was the one who requested it) but it should be only enabled in production as not to have warnings everywhere - at least if there is no fix in the horizon-.

It seems like a PR has been submitted to silence the warning. Even freaking @LeaVerrou chimed in on this.. So yeah, I'm not sure if hiding all warnings is a solution but I guess it could be one for the time being at least..

@varoot
Copy link

varoot commented Nov 13, 2017

I don't think we should just suppress the warning. Having postcss-custom-properties is useful for supporting older browsers since it doesn't break supporting browsers. I think the problem is that component css files are compiled separately from global css file (and also before). If we can concat the global and component css files (put global styles first) then postcss should be able to read the variables and compile correctly.

@cyrilletuzi
Copy link
Contributor

cyrilletuzi commented Dec 1, 2017

The problem is more serious than the issue title.

Here are details on what @varoot explained :

If you serve your app in AOT with ng serve --aot, there won't be a warning anymore. But the feature won't work at all.

Given this code in styles.scss :

:root {
  --my-color: red;
}

And this code in a component :

p {
  color: var(--my-color);
}

the final CSS produced will be :

p {
  color: var(--my-color);
  color: var(--my-color);
}

But if I use the CSS var in styles.scss it will be OK :

p {
  color: red;
  color: var(--my-color);
}

So it's like the CSS variables declared in styles.scss are not taken into account when parsing components styles.

The issue should be renamed and silencing the warnings won't solve the problem.

I also don't understand @clydin proposition :

The most appropriate solution would be to only enable the plugin when the app needs to support browsers that do not have native css variable support.

The problem is here in all cases.

So currently this feature doesn't have a lot of sense, as being limited to use CSS variables just inside styles.scss is not useful. CSS variables would be useful to use in components style, for theming.

@garoyeri

@varoot
Copy link

varoot commented Dec 1, 2017

@cyrilletuzi
I think the reason why a lot of people suggested making the plugin optional is that it works at run-time because then you'll have your global styles (styles.scss) together with component styles so the variables are resolved correctly.

Maybe it would be solves by just making sure global styles are included before the component styles in the webpack config webpack-configs/styles.ts?

@cyrilletuzi
Copy link
Contributor

It will work for browsers supporting CSS variables, but not for old browsers. Then what's the point of the postcss-custom-properties feature ? Either we can make it work correctly, either we may revert this feature completely.

@cyrilletuzi
Copy link
Contributor

See this comment in the initial PR

Ok, I did a little more digging, turns out that it is completely unsupported by postcss-custom-properties as per the issue here: postcss/postcss-custom-properties#68.

Basically, the plugin only operates on a single file unless you use postcss-import to inline the other styles which may cause weird things to happen.

@garoyeri
Copy link
Contributor

garoyeri commented Dec 5, 2017

I agree with @cyrilletuzi , it really should be done right or we need to roll it back out. I didn't understand enough about how the CSS build chain worked and I presumed my PR would work since it covered all my use cases.

The warning will show up since the variable was not defined. Also, it would be good to make it an optional step instead to enable support for IE11. Otherwise there's no reason to have this feature. Since the other Polyfills are brought in optionally, this, too, should be.

@clydin
Copy link
Member

clydin commented Dec 5, 2017

As I mentioned in my previous comment, the plugin can only work if it has full knowledge of the CSS variables at build time. The plugin is a build time processing step that alters the stylesheet to mimic the behavior of CSS variables (in certain situations). The plugin, therefore, cannot work if the variables are split between global and component styles as they exist in different contexts at build time.

The plugin should also not be used if only browsers with native support are used since the browser can support the full spectrum of css variable usage. This is actually how cssnext uses the plugin (via a browsers list check).

@garoyeri
Copy link
Contributor

garoyeri commented Dec 5, 2017

My first shot at the PR was to use cssnext instead, but it pulled a LOT of dependencies, some of which were licensed in ways that precluded us from using them in Angular CLI (e.g. public domain).

@mgol
Copy link
Member

mgol commented Dec 7, 2017

I agree in its current state the plugin doesn't make much sense for Angular CLI projects. Component styles would be able to transform CSS props only if they themselves included definitions of the variables on :root. Doing that in components would be an anti-pattern, though.

Therefore, in projects following best practices the only thing the plugin does in component styles is produce lots of warnings during the build and duplicate CSS rules during development (which clutters the view in DevTools).

@varoot
Copy link

varoot commented Dec 9, 2017

Let me see if I understand this accurately

  1. The plugin currently only works if the css variable is defined in the :root on the same file where it's used (not even when it's imported).
  2. Defining :root also doesn't work properly on component stylesheet if the view is encapsulated (but the plugin won't complain)

So basically this plugin is only useful for people who wants to support legacy browser and only have one big global stylesheet (or define variables in each of the global stylesheets where it's use)

That really doesn't seem very useful and it might be better to just remove it.

postcss-custom-properties has variables option (https://github.com/postcss/postcss-custom-properties#variables). Maybe we can leverage this and define all css variables in .angular-cli.json or something then only enable this plugin when the variables are defined? (We will also need to output those variables in a :root selector if we go that route)

filipesilva added a commit to filipesilva/angular-cli that referenced this issue Dec 14, 2017
We currently use a fallback for CSS custom properties on older browsers. This approach has a few problems:
- does not work if the custom property declaration is not part of the same of the same file that uses it (multiple global stylesheets, component css).
- does not work at all for component CSS in AOT.

@clydin suggested a browserlist based approach for enabling this functionality, but that requires a new feature that we don't have.

Since currently taking advantage of the custom property fallback is flaky even in the best case scenario, and potentially broken in prod (AOT), I think it's better to remove it altogether until we can actually do it right.

Fix angular#8289
filipesilva added a commit to filipesilva/angular-cli that referenced this issue Dec 14, 2017
We currently use a fallback for CSS custom properties on older browsers. This approach has a few problems:
- does not work if the custom property declaration is not part of the same of the same file that uses it (multiple global stylesheets, component css).
- does not work at all for component CSS in AOT.

@clydin suggested a browserlist based approach for enabling this functionality, but that requires a new feature that we don't have.

Since currently taking advantage of the custom property fallback is flaky even in the best case scenario, and potentially broken in prod (AOT), I think it's better to remove it altogether until we can actually do it right.

Fix angular#8289
@ChristianSchroedel
Copy link

Has this already been fixed in a release?

We still get lots of these warnings in v1.6.1.
Also I cannot find @filipesilva fixes in the current master branch of the angular/cli project.

filipesilva added a commit to filipesilva/angular-cli that referenced this issue Jan 9, 2018
We currently use a fallback for CSS custom properties on older browsers. This approach has a few problems:
- does not work if the custom property declaration is not part of the same of the same file that uses it (multiple global stylesheets, component css).
- does not work at all for component CSS in AOT.

@clydin suggested a browserlist based approach for enabling this functionality, but that requires a new feature that we don't have.

Since currently taking advantage of the custom property fallback is flaky even in the best case scenario, and potentially broken in prod (AOT), I think it's better to remove it altogether until we can actually do it right.

Fix angular#8289
clydin pushed a commit that referenced this issue Jan 9, 2018
We currently use a fallback for CSS custom properties on older browsers. This approach has a few problems:
- does not work if the custom property declaration is not part of the same of the same file that uses it (multiple global stylesheets, component css).
- does not work at all for component CSS in AOT.

@clydin suggested a browserlist based approach for enabling this functionality, but that requires a new feature that we don't have.

Since currently taking advantage of the custom property fallback is flaky even in the best case scenario, and potentially broken in prod (AOT), I think it's better to remove it altogether until we can actually do it right.

Fix #8289
clydin pushed a commit that referenced this issue Jan 11, 2018
We currently use a fallback for CSS custom properties on older browsers. This approach has a few problems:
- does not work if the custom property declaration is not part of the same of the same file that uses it (multiple global stylesheets, component css).
- does not work at all for component CSS in AOT.

@clydin suggested a browserlist based approach for enabling this functionality, but that requires a new feature that we don't have.

Since currently taking advantage of the custom property fallback is flaky even in the best case scenario, and potentially broken in prod (AOT), I think it's better to remove it altogether until we can actually do it right.

Fix #8289
@skalinets
Copy link

Just for reference. Bulma maintaner have added a variable that can remove those wornings, $variable-columns: false;. Here are more details: jgthms/bulma#1190 (comment)

dond2clouds pushed a commit to d2clouds/speedray-cli that referenced this issue Apr 23, 2018
We currently use a fallback for CSS custom properties on older browsers. This approach has a few problems:
- does not work if the custom property declaration is not part of the same of the same file that uses it (multiple global stylesheets, component css).
- does not work at all for component CSS in AOT.

@clydin suggested a browserlist based approach for enabling this functionality, but that requires a new feature that we don't have.

Since currently taking advantage of the custom property fallback is flaky even in the best case scenario, and potentially broken in prod (AOT), I think it's better to remove it altogether until we can actually do it right.

Fix angular#8289
@raphaelokon
Copy link

raphaelokon commented Jun 7, 2018

We depend on postcss custom properties and need it included in our build → What is the proposed workaround for adding own postcss plugins to an angular-cli project?

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful type: bug/fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.