Skip to content

fix(@angular/cli): remove postcss-custom-properties #8874

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

Merged
merged 1 commit into from
Jan 9, 2018

Conversation

filipesilva
Copy link
Contributor

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

@filipesilva filipesilva self-assigned this Dec 14, 2017
@filipesilva filipesilva requested a review from clydin December 14, 2017 14:25
Copy link
Contributor

@Brocco Brocco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small thing

@@ -67,7 +66,7 @@ export function getStylesConfig(wco: WebpackConfigOptions) {
// If deployUrl contains a scheme, ignore baseHref use deployUrl as is.
return `${deployUrl.replace(/\/$/, '')}${url}`;
} else if (baseHref.match(/:\/\//)) {
// If baseHref contains a scheme, include it as is.
// If baseHref contains a scheme, include it as is.git
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove random git text.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Urf durf, my bad

@byrondover
Copy link

👏

Brocco
Brocco previously approved these changes Jan 8, 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 clydin merged commit c53dc67 into angular:master Jan 9, 2018
@filipesilva filipesilva deleted the remove-custom-props branch January 9, 2018 16:26
@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 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

css warning "variable '--some-var' is undefined"
7 participants