Problem/Motivation
While implementing the new design system, the team raised some interest in using some of the new CSS features (mainly CSS custom properties or CSS variables). Being able to leverage these new features would lead to less overhead in the maintenance.
For example, CSS custom properties are currently supported by latest browser versions, while some older browsers are lacking support (browser support matrix). However, with the usage of CSS preprocessor, we could already reliably use CSS custom properties.
Proposed resolution
Use PostCSS to compile a version of our CSS that is supported by the browsers Drupal currently support. PostCSS is used by industry leaders including Wikipedia, Twitter, Alibaba, and JetBrains. PostCSS has over 20 000 stars in Github and is being used by almost 2 million projects according to Github.
Plugins we are currently using:
- Autoprefixer is a plugin to parse CSS and add vendor prefixes to CSS rules using values from Can I Use. It is recommended by Google and used in Twitter and Alibaba.
- PostCSS Calc reduces calc() references whenever it's possible. This can be particularly useful with the PostCSS Custom Properties plugin.
- PostCSS Custom Properties lets us use Custom Properties in CSS, following the CSS Custom Properties specification.
- PostCSS Inline plugin to transform @import rules by inlining content. This enables importing variables from a central file.
Dependency evaluation:
- Maintainership of the package: PostCSS project itself has multiple maintainers but it seems that only one of them is active in the core. Full list of maintainers. PostCSS repositories are active and PR's seem to receive feedback very quickly. PostCSS is used by at least 2.7 million other packages. PostCSS is also used by some large organizations including Wikipedia, Twitter, Alibaba, and JetBrains.
- Security policies of the package: Security issues can be reported privately using Tidelift. This library is only used in by core developers in development environments.
- Expected release and support cycles: Looking at the history of the library, there has been a new major release every 2 years. Only single major release is supported at the time.
- Code quality: APIs are well documented and the library has sufficient test coverage.
- Other dependencies it would add, if any: list of additional dependencies
Development workflow Q&A
How would this affect the developer workflow?
Core developers will have to run yarn run build:css
while making changes to CSS. At first, this will only affect CSS in Claro but eventually, given that the tooling is there, this is something that could be expanded outside of Claro.
So now I need to run not only yarn run watch:js
but also yarn run watch:css
, doesn't this harm DX?
If you're working on a theme using PostCSS and using ES6 JS, then you can just do yarn run watch
to do both yarn run watch:js
and yarn run watch:css
at the same time 👍
Would it affect only core developers, or would there be additional required steps for contrib or sites as well?
This tooling is designed to be specific to core and doesn't affect contrib in any way.
Would we handle it the way we do ES5 transpilation, with patch contributors responsible for including both the source and the compiled version, and committers' toolchains verifying it on commit?
Yes.
What's the plan for the dependency if Claro is not added to core?
This is indeed added mainly Claro in mind, but this isn't specific to Claro in any way. For example, the new frontend theme (#3064880: Create new default front-end theme) is planning to use PostCSS as well.
Remaining tasks
None.
Release notes snippet
N/A
Comment | File | Size | Author |
---|---|---|---|
#64 | interdiff.txt | 523 bytes | lauriii |
#64 | 3060153-64.patch | 21.85 KB | lauriii |
#61 | 48-60-interdiff.txt | 457 bytes | alexpott |
#60 | 3060153-2-60.patch | 22.71 KB | alexpott |
#60 | 48-60-interdiff.txt | 22.71 KB | alexpott |
Comments
Comment #2
chicagomom CreditAttribution: chicagomom as a volunteer commentedUsing PostCSS could provide an additional opportunity: using the PostCSS Style Guide plugin (Github) to generate style guides using markdown. There have been several discussions concerning the need for a live style guide for the admin theme (and perhaps all themes) using a variety of proposed methods. For more background:
Comment #3
AaronMcHaleRe #2: @2chicagomom big +1 to that, I’m always a fan of using automation to improve processes and a living automated style guide would be a great way to reduce manual work, as well as guarantee it’s always up to date.
Comment #4
mherchel+1 to this. We hope to use PostCSS on a new Drupal core front-end theme, too.
Comment #5
catchHow does this impact people who need to make a one-off change to core CSS? Do you need to build the CSS and add the changes to the patch?
My main experience of CSS post-processors, because I hardly ever do front-end development, is having to navigate out-of-date instructions on what to run to build the CSS, running into mis-matching versions etc. - how susceptible is postcss to that kind of problem?
Comment #6
lauriiiThe impact would be comparable to the process we currently have for JavaScript. People would have to run a command while working on a patch. I don't think we have another way to handle this since we don't have a build process where we could add this easily.
I've run into similar issues but it's only been when the stack has been built on Ruby which was for a long while the most commonly used language for compiling frontend assets. However, we are using JavaScript so the tooling is similar to what we already use for our core JavaScript, including the package manager. There shouldn't be issues with out-of-date instructions since there are just two commands people generally have to run, an they're both controlled by us (again similar to what we have for JavaScript). This also means that even if our external dependencies change their commands, that wouldn't affect our users.
Comment #7
gueguerreiroUpdating the issue summary to fix malformed links.
Comment #8
xjmLet's put the dependency evaluation in the IS too. Some of the points are already covered by the proposed resolution, but not all of them.
Comment #9
alexpottI'm +1 to using the same modern build tools that everyone else is using to build frontends. Especially as the selected tool is using the same tools to manage dependencies as our existing JS build process. Also as this is only for development and does not affect Drupal's runtime code there are less security concerns than we have with adding new PHP dependencies.
I agree with @catch that often the tooling and build instructions can cause problems however I'm hopefully that as we're going to build on our existing devtools and commit the final CSS files (like we do for JS) we can mitigate this.
Comment #10
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedI have no strong opinion on PostCSS itself (or any other preprocessor), but I'm very interested in the automatically updated living styleguide mentioned in #2.
@chicagomom - is the generated styleguide a real HTML document using the resulting stylesheet? As opposed to generated screenshots from a headless browser, say.
If the styleguide is real HTML then it gets a big +1000 from me. Such living styleguides are very handy for manual accessibility review and regression testing; we can point a checker like aXe at them, or load them in a browser to for manual testing of focus styles, WCAG resize/reflow criteria, etc.
Comment #11
droplet CreditAttribution: droplet commented#10
Not that smart but increasing work on both the patcher or reviewer. The styleguide is a generator for human annotations code and required a modified version to adopt the PHP code (via Drupal FORM API or theme functions ..etc.) to generate more recent HTML code.
There's a very common case in Drupal. The extra CSS files & CSS classes loaded from JS. And multiple CSS files from the CORE system, modules, stable theme until your actual theme. This doesn't end... Sometimes, you were using a child theme. You need a combination to get the final style.
The styleguide could be a quick demo for the themer to take a glance at different element components. e.g.: What's the shape and color for a warning button? But not for fully accessibility review or any patch reviewers.
With the limitations, to build a standard module to loop over all Drupal FORM API & Theme components in a single page is more efficiently. A system like storybook is advanced:
https://github.com/storybookjs/storybook
one small step for Claro, one giant leap for Drupal CORE
#2658650: [META] Optimize Frontend Workflow in Core Development
Comment #12
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commented#11: that doesn't answer my question. Does the generated styleguide have screenshots of the components, or HTML?
Are there any examples of styleguides I can try, generated with the plugin mentioned in #2? The GitHub README doesn't say so.
I've don't think I've ever heard the term "warning button" before. I'm not sure what one of those is.
Comment #13
lauriii#12: the styleguide will have HTML and CSS rather than screenshots. However, I think we should review details related to that in a follow-up. There's strong arguments in favor of using PostCSS even if we didn't use it for generating styleguides.
Comment #14
alexpott@catch and I are fine with this from a framework manager point of view. As per #9 I've outlined my reasons for why,
I think now we need to work on dependency evaluations as per #8 in order to satisfy the release manager review requirement.
Comment #15
lauriiiComment #16
lauriiiComment #17
lauriiiComment #18
xjmA few questions:
This would need a CR, and specific instructions for how it'll get used in our process it would be good as well.
Thanks everyone!
Comment #19
xjmAlso, if we're going to add new things to the committer dev toolchain, let's please do that well in advance of alpha and have complete instructions/roadmap/etc.
I could see committing Claro as alpha with just the compiled CSS, and using PostCSS as internal development tool until we're ready to go beta. Or we could add both as alpha, with them needing an
rm -rf
from 8.8.x. Just please don't change the committer workflow or toolchain during October.Comment #20
xjmOh, we also need to evaluate the dependency tree, not just the dependency. That's a whole lot of dev dependencies. (I mean, it's par for the course in the JS world, but not something that's in Drupal core process yet outside the JS initiative.) So it's an important step that we should discuss and think about.
Would we be using the tool as a dev build or a prod build in our workflows? If it's a prod build, cool, just evaluate those other three dependencies. But if we get all those dev dependencies and any one of them can break our development build... well, that's us entering the scary JS zillion-dependency world. So we should also talk about what the consequences would be if we had a leftpad incident -- or malicious code -- in one of the dependencies.
Comment #21
xjmOr, framed another way, let's document a sample
yarn.lock
of the build we'd use, and list the dependencies there, and document/evaluate them at least at a high level.Comment #22
xjmOops, I realized one of my questions (about the workflow being like ES5 transpilation or not) was already answered by @lauriii in #6. Sorry! This goes back to the need for the CR and IS update. :)
Comment #23
lauriiiHere's an attempt on a proposed workflow. This is very close to what we already have in place for ES6. In fact, I copied the architecture of their compilation process directly into this. This compiles *.pcss.css files into .css files and inserts a header.
These are the new dev dependencies added for Drupal Core as part of this (can be seen in the yarn.lock file:
Comment #24
lauriiiCleaned up some documentation and moved packages accidentally added as production dependencies to dev dependencies.
Comment #25
xjmThe patch is a huge help; thank you.
So, if I'm reading right, these are the first non-dev dependencies in our core
package.json
. The rest, eslint and friends, are devDependencies. Is there a reason for the difference, and does it impact anything?Also, just to confirm: While they're listed as prod dependencies, that doesn't (?) mean prod for sites with their own JS dependency management. These will only end up on their servers if they go into the core dir and
<del>npm</del> yarn install
, which they have no reason to do unless they're specifically working on core. Correct?(One of the things we're always trying to evaluate is risk and exposure. Every dependency is a security risk, and so it's important to know how many laptops and servers might intentionally or unintentionally end up with new things installed.
Thanks!
Comment #26
xjm#24 answers most of #25. Brain waves in synch!
Comment #27
xjmOkay, great, so it's actually adding very little besides itself and other things we use for like eslint and stylelint and the transpilation. That makes it a much more manageable thing to evaluate and shows the disruption to the dep tree is small. Thanks!
In this, if I've read correctly, the only new library outside postcss libraries is autoprefixer.
So, let's do a dependenct evaluation for that one too. Thanks for your patience as i muddle through JS dep management things. :)
Oops, the needs followup tag was to separate documentation of core development toolchain vsl other dev res snd specific policies for them on https://www.drupal.org/core/dependencies#dev
Comment #28
lauriiiThe reason for not listing autoprefixer in #23 is that we already have implicit dependency on autoprefixer through stylelint. I'm happy to provide an evaluation on that as well if we think that would be useful ✌️
Comment #29
finnsky CreditAttribution: finnsky at Skilld commentedNice initiative. +1
I found some places where we can to optimise things:
1) For example some scripts for js and css are equal. Like ChangeOrAdded.js
Difference only in file ext. So we can merge common scripts.
2) We have few environments settings in package json now. It looks wierd imo. Also we have browserlist config line but `browserlist` not added in dependencies. Much better will add it in dependencies. use its power in environments configuration. and remove that settings from package.json.
https://github.com/browserslist/browserslist#configuring-for-different-e...
Comment #30
finnsky CreditAttribution: finnsky at Skilld commentedUpdated patch with described in #29 approach.
Comment #31
alexpottThanks for the suggestions @finnsky. I think the refactors make sense in someways but doing them should be a separate issue so we can discuss and BC concerns and other requirements. Also @lauriii points out that #30 tightly couples the CSS and JS build process. While CSS is brand new perhaps that will paint us into a corner we don't want to be in.
Comment #32
lauriii#29.1 I kept the CSS build process separate from the JS on purpose. Having them defined as separate pipelines allows us to make changes to these two build tools individually. If we make both build tools to use the same abstracted tools, it forces someone making changes to CSS build to also have to worry about the JS build tools. These are also fairly simple files so the overhead from duplicating this code is not that high.
#29.2 I removed the duplicated browserslist configuration but I'm not sure we should be defining an explicit dependency on browserslist since we are not actually directly depending on it anywhere.
Comment #33
finnsky CreditAttribution: finnsky at Skilld commentedRE: #32 #31 Actually build processes decoupled. Merged only file system processes. Both js and css build tools have own config and plugins list. And still independent from each other.
Comment #34
lauriii@finnsky Do you think we could open a follow-up to discuss the abstractions part?
Comment #35
finnsky CreditAttribution: finnsky at Skilld commentedRe: #34
Yes. Because build/watch are filesystem tasks. No matter what they build. Css/js/images/icons etc.
---
Also would be nice to create follow-up about usage of .babelrc and postcss.config.js to disquss abilities of modules and themes to use own plugins and configs with core build tools.
Thanks.
Comment #36
lauriii@finnsky This might be the issue you're looking for: #2957390: Use ES6 for contrib and custom JavaScript development.
Comment #37
lauriiiOpened follow-up #3084850: Consider abstracting some parts out of JavaScript / CSS build tools for the abstraction changes proposed in #29.
Comment #38
lauriiiFollow-up opened for #27: #3084853: Document development dependencies.
Comment #39
lauriiiComment #40
lauriiiComment #41
lauriiiOpened change record.
Comment #42
Wim Leers👍
The list of targeted browsers remains the same, it is just being moved. Rather than it being passed specifically to
babel
only, we're making this the officialbrowserslist
that any JS package is being told to target.Why are we doing this? Because
autoprefixer
also uses this.👍
This is the only remotely concerning JS dependency, because it has fewer installs/users. But … it's absolutely trivial, so the potential burden on us (the Drupal community) to maintain this ourselves is minimal and trivial.
We want/need this for the "DO NOT EDIT THIS FILE" header at the top of the CSS files built/processed by PostCSS (
*.pcss.css
→*.css
). Just like we do for JS files (*.es6.js
→*.js
).🤔 These files are 100% identical to the identically named files in
core/scripts/js/
. Any way we can reuse rather than duplicate?🤔 Let's document the reasons for these flags.
👍 The reason for this is obvious.
🤔
This is the only change, can we make this show up as a copy + modify in thegit
-generated patch?🥳 Found it! Generate a patch using
git diff -C -C
(that is not a typo). This does not make the patch smaller, but does it make it significantly easier to review, because it makes it very obvious that this code is following an already established pattern.👍 A lot of additions here, but
yarn why
explains all of them: they are dependencies of thepostcss
packages we're adding.yarn run watch:css
, which is similar toyarn run watch:js
. But what if I want both at the same time? I want a single command to watch and build all the things.Comment #43
Wim LeersMarking
for points 3, 4 and 8.Comment #44
lauriiiThank you for the review @Wim Leers 🙏
#42.3 We already have a follow-up for this: #37.
#42.4 👍
#42.8 👍
Comment #45
Wim Leersyarn run watch
and not alsoyarn run build
?postcss-import
.Why are we adding this now?Oh you updated the issue summary! 👏Comment #46
lauriiiGood idea! Added
yarn build
as well.Comment #47
Wim Leers#46: Excellent! Now I can just change that terminal tab from
yarn run watch:js
toyarn run watch
and still have everything being built for me! 🥳I took one last hard look at this issue. I found only one remaining question before I'd RTBC this:
yarn run watch:all
andyarn run build:all
would be clearer? Are there conventions for this? I'm fine with this as-is, but if there is a front-end developer community convention around this, we should go with that.Comment #48
lauriiiI've never seen
yarn run watch:all
myself. 🤔 I made a quick Google search with keywordssite:github.com "yarn watch"
andsite:github.com "yarn watch:all"
which confirm that there seems to be only a handful of libraries usingyarn watch:all
. In comparison, there's tens of pages that can be found with the search looking foryarn watch
. I'm personally fine to go either way, butyarn watch
felt like the natural way to provide a watch-all-the-things command.Comment #49
Wim LeersSounds good!
I expanded the issue summary to preemptively answer the committer question
🤓Comment #50
Wim LeersThis is no longer just a plan, nor is it tightly coupled to Claro. The issue summary already reflects this. The issue title did not yet.
Comment #51
Wim LeersThis is a blocker for #3079738: Add Claro administration theme to core.
Comment #52
xjmThe CR should mention that, for the time being, this only applies to the new experimental Claro theme. Otherwise it sounds way more disruptive than it is.
We should also add a PR to the committer toolchain to check claro theme files (once they exist) in the pre-commit hook and verify the build output matches, as is done with JS.
Comment #53
xjmAlso, I need links to and documentation of Autoprefixer's security policies/contacts, release policies, even the repo, etc. Thanks!
Comment #54
xjmComment #55
lauriiiI updated the CR.
#53: Autoprefixer is part of the PostCSS toolchain. They follow the same policies as the main repository and they also use Tidelift for reporting security issues.
Still need to open PR for the committer tooling.
Comment #56
lauriiiIf anyone else is interested in working on the committer tooling improvement, the repository for that is here: https://github.com/alexpott/d8githooks .
Comment #57
Wim Leers#56: On it.
Comment #58
Wim LeersDone: https://github.com/alexpott/d8githooks/pull/24
Comment #59
Wim Leershttps://github.com/alexpott/d8githooks/pull/24 was merged already 😳
Comment #60
alexpott@Wim Leers's merge on githooks revealed this patch has a tiny issue :)
Comment #61
alexpottughh wrong interdiff.
Comment #62
lauriiiI was planning to remove these changes from the patch before this actually gets committed. This was here just for demonstration purposes. I guess the previous comments prove that it works as expected?
Comment #63
alexpottYep! I thought I'd just learned that we already had a .pcss.css file in HEAD :D lolz... okay yeah we should be removing that and the changes to seven's css. And also yay @Wim Leers's update to githooks look like they work fine.
Comment #64
lauriiiNot sure why removing the .pcss.css file doesn't show up in the interdiff. 🤯Moving back to RTBC since my changes are insignificant.
Comment #65
Wim Leers🥳
Comment #66
alexpottAs far as I can see all the release manager feedback has been addressed and this is ready for commit. Crediting @xjm and @catch for release management comments. Will wait for them to confirm or remove the tag before committing.
Comment #67
catchI think all of xjm's feedback has been addressed, so removing the needs release manager review tag.
Comment #68
alexpottCommitted 0bada78 and pushed to 8.8.x. Thanks!
Comment #70
Wim LeersPublished the change record! 🥳
Comment #71
davidhernandezShould this be marked fixed without a full documentation page in the contribution guide? This is the first of its kind.
Comment #72
saschaeggiOnce in core, can we also start using the nesting plugin for postCSS (SCSS-like syntax):
https://github.com/jonathantneal/postcss-nesting
That could also save a lot of time rewriting CSS.
Comment #73
catchBack to needs work until the contributor documentation is up.
Comment #74
droplet CreditAttribution: droplet commented.pcss.css
isn't a well-compatible file extension for popular editors, eg. VSCode & PHPStorm. While this is still fresh, I think the community should make a good decision immediately to reduce code debt.Comment #75
lauriii#74: Code editors read
.pcss.css
as.css
by default. We are using PostCSS for generating CSS that's supported by older browsers from modern CSS. This means that we don't really need any special IDE handling for these files in core. Since the tooling is internal and designed to be used only by core, contrib projects willing to use PostCSS for something else could continue to use any other file extension as they wish.Comment #76
xjmHmm, why doesn't this reflect our new browser policy? (as I reviewed it in another issue recently).
Also I heard someone say in backchannel that this patch uses an unsupported release if PostCSS; is that true? If so we need to discuss that and possibly revert, or get more info from the maintainers about the issue with the current version.
Comment #77
lauriiiWe have an issue to solve that #3084843: Update browserslist browsers. It's not in the scope of this issue.
Core is not using an unsupported version of PostCSS, or any other libraries related to this. Claro is using a previous major version of one of the libraries and there was a regression on the most recent major release. I've opened an issue in the package to solve that and we've come up with a workaround for the problem in Claro. Before this research happened, it was on the table to use an older version of that library in core as well, but that never happened because we don't want to depend on an unsupported version of a library, and we already found a better workaround for the problem.
Comment #78
droplet CreditAttribution: droplet commented#75
Am I right this only enables CSS4 CUSTOM VAR?
I've already seen suggestions coming up #72.
this config isn't really used? If not, `importFrom` should not specify for CORE
Comment #79
davidhernandezComment #80
lauriiiWe use it for autoprefixer and CSS custom properties, calc and import. None of this requires any special handling from IDEs.
😱That's a mistake. It's not actually needed. Let's open a follow-up to remove that.
Comment #81
zrpnrI wasn't sure where to add this documentation, I found Javascript Developer tools for Drupal core which already had info about installing yarn and running the js build commands.
I added the new commands for css as well as some intro text to explain JS and CSS preprocessing. I copied some passages from the respective change records as well since they had good examples of how and why to run these commands.
It might be better to have a separate page, but on the other hand this is part of the same tooling and managed the same way as js.
#80
Opened a followup to remove the claro path in postcss.config.js
#3086931: Remove unused postcss.config.js
Comment #82
lauriiiI moved the documentation under Drupal 8 from JavaScript coding standards. I also changed the title to frontend development tools which is more accurate. Marking this as fixed since #79 and #80 has been addressed.
Comment #83
xjmAh, part of the needed documentation was for the dependency itself, on this page:
https://www.drupal.org/core/dependencies#dev
Thanks!
Comment #84
lauriiiAdded documentation to the core dependencies documentation! Moving back to fixed.
Comment #86
jhodgdonI found small a bug in the way this was done and created a child issue; posting here even though the issue is closed, in case anyone is following this.
#3155159: Fix notice at top of generated CSS files from PCSS