-
Notifications
You must be signed in to change notification settings - Fork 361
Add support for CSS Modules #2431
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
Add support for CSS Modules #2431
Conversation
… Modules (must come before "styles()" in order to avoid processing errors).
|
Size Change: +48 B (+0.01%) Total Size: 472 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (4e70682) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2431If you are working in Khan Academy's frontend, you can run the below command. ./tools/bump_perseus_version.js -t PR2431If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -s n -t PR2431 |
…y reference CSS Modules files.
…add layer statements to new CSS files instead of the old LESS files.
…o pre-built CSS files to simplify our styling framework
…SS conversion branch.
…postcss plugin configuration.
…uring conversion. Handle interpolated strings during conversion.
…nality for BinaryExpression and ObjectExpression.
…r to WB color variables. Add conversion of media queries.
…ects. Add ability to map variables loaded from node_modules.
…dules for styling, and a converter for Aphrodite code
| @@ -0,0 +1,3 @@ | |||
| .css-modules-test { | |||
| opacity: 0.99; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for demonstration purposes only (to show that CSS Modules can be imported). It will be replaced with widget-specific styling from the Aphrodite conversion in a future PR.
| const path = require("path"); | ||
|
|
||
| const {parse} = require("@babel/parser"); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is much longer that I would normally like things like this to be. It is intended to be temporary, but if folks prefer that this be broken up into smaller files, I'm happy to oblige. I'm happy to pair with anyone to explain what is happening in any section of the code. Also, since this is intended to be temporary, I didn't make it a TypeScript file, nor did I write any tests for it. It is intended to be a tool to aid in our conversion process. Let me know if you have strong feelings for a different approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at a minimum it'd be good to put a JSDoc comment on the top explaining how to use/invoke it and what it does (reads a component file and dumps a .css file?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ivyolamit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks logical to me 👍🏼 Thanks for creating the script to make it easier 😉
jeremywiebe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the way this is moving. One major thing that I think should be figured out before we say we're fully on CSS Modules is tooling support (in a future PR, I mean). Right now, there is no linting/"type checking" to ensure that the class names used from a CSS module actually exist. That feels like it'll become a bigger and bigger footgun over time.
| plugins: [postcssImport()], | ||
| modules: { | ||
| localsConvention: "camelCase", | ||
| generateScopedName: function (name, filename, css) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment here explaining what this is doing? I think it's building a CSS classname that is prefixed with perseus_ and then is the hash of the CSS file contents... but it'd be good to explain for me in the future. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
pnpm-workspace.yaml
Outdated
| tiny-invariant: 1.3.1 | ||
| underscore: 1.4.4 | ||
| vite: 5.1.0 | ||
| vite-css-modules: 1.8.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really need to put this in the catalog if it's only used in one package.json. The catalog really shines where we have the same dep showing up in multiple places in the repo and we want to keep them all on the same version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved back.
| const path = require("path"); | ||
|
|
||
| const {parse} = require("@babel/parser"); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at a minimum it'd be good to put a JSDoc comment on the top explaining how to use/invoke it and what it does (reads a component file and dumps a .css file?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some usage comments (none of which should block this PR and may all be fine to leave as-is because of the short lifespan of this tool):
- I ran
node utils/extract-css.js packages/perseus/src/components/hud.tsx:- It didn't add an import that was needed:
import {css} from "aphrodite"; - It left an unused import in place:
import * as constants from "../styles/constants";
- It didn't add an import that was needed:
- The export from the CSS module file doesn't seem to have any type info which will mean that TypeScript won't help us verify that we have typed class names correctly. This is probably something we'll want to investigate and figure out in the near future.
- IMHO, you could just over-write the original file being processed. This is a git repo and so
git diffwill show exactly the changes. 🤷♂️ - For an stress-test, I ran this across all .tsx files in the repo 👿 It hit several issues in the script (several "cannot access XYZ on undefined" issues).
git ls-files packages/**/*.tsx | xargs -n 1 -t node ./utils/extract-css.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- JSDoc comment added to file.
- RE: Ran on
hud.tsx- I anticipate the need to add more functionality as other files are converted.
- I don't remove imports of external files since it's possible that those objects/values are used elsewhere. I might add a search function in the future to check to see if such imports are no longer used.
- Type info added. Typescript now understands the imported CSS Modules and will cause a failure if code is trying to reference a class name that doesn't exist.
- Added an
--archiveoption to the script command. File archiving is "off" by default. - RE: Stress test
- See item 2 (I'll address issues as they arrive).
…ig to clarify output of generateScopedName. Add TypeScript parsing of CSS Modules files. Make TSX file archiving optional (default to off). Auto-add CSS Modules file to git changes.
| preprocessorOptions: { | ||
| less: { | ||
| math: "always", | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right. Less is gone!
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/kas@2.0.7 ### Patch Changes - Updated dependencies \[]: - @khanacademy/perseus-utils@2.0.4 ## @khanacademy/keypad-context@3.0.16 ### Patch Changes - Updated dependencies \[]: - @khanacademy/perseus-core@14.0.3 - @khanacademy/perseus-utils@2.0.4 ## @khanacademy/kmath@2.0.16 ### Patch Changes - Updated dependencies \[]: - @khanacademy/perseus-core@14.0.3 - @khanacademy/perseus-utils@2.0.4 ## @khanacademy/math-input@26.0.5 ### Patch Changes - Updated dependencies \[]: - @khanacademy/keypad-context@3.0.16 - @khanacademy/perseus-core@14.0.3 - @khanacademy/perseus-utils@2.0.4 ## @khanacademy/perseus@65.1.2 ### Patch Changes - [#2431](#2431) [`218eb4cb1`](218eb4c) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Add CSS Modules for styling, and a converter for Aphrodite code - [#2603](#2603) [`c2a31923c`](c2a3192) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Remove findDomNode from Radio Widget as it is deprecated. - Updated dependencies \[]: - @khanacademy/kas@2.0.7 - @khanacademy/keypad-context@3.0.16 - @khanacademy/kmath@2.0.16 - @khanacademy/math-input@26.0.5 - @khanacademy/perseus-core@14.0.3 - @khanacademy/perseus-linter@4.0.3 - @khanacademy/perseus-score@7.1.3 - @khanacademy/perseus-utils@2.0.4 - @khanacademy/pure-markdown@2.0.7 - @khanacademy/simple-markdown@2.0.7 ## @khanacademy/perseus-core@14.0.3 ### Patch Changes - Updated dependencies \[]: - @khanacademy/kas@2.0.7 - @khanacademy/perseus-utils@2.0.4 ## @khanacademy/perseus-editor@24.0.3 ### Patch Changes - [#2597](#2597) [`0c2beadca`](0c2bead) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph] Update autogen labels to correctly reflect locked line vs segment vs ray - Updated dependencies \[[`218eb4cb1`](218eb4c), [`c2a31923c`](c2a3192)]: - @khanacademy/perseus@65.1.2 - @khanacademy/kas@2.0.7 - @khanacademy/keypad-context@3.0.16 - @khanacademy/kmath@2.0.16 - @khanacademy/math-input@26.0.5 - @khanacademy/perseus-core@14.0.3 - @khanacademy/perseus-linter@4.0.3 - @khanacademy/perseus-score@7.1.3 - @khanacademy/perseus-utils@2.0.4 - @khanacademy/pure-markdown@2.0.7 ## @khanacademy/perseus-linter@4.0.3 ### Patch Changes - Updated dependencies \[]: - @khanacademy/perseus-core@14.0.3 - @khanacademy/perseus-utils@2.0.4 ## @khanacademy/perseus-score@7.1.3 ### Patch Changes - Updated dependencies \[]: - @khanacademy/kas@2.0.7 - @khanacademy/kmath@2.0.16 - @khanacademy/perseus-core@14.0.3 - @khanacademy/perseus-utils@2.0.4 ## @khanacademy/pure-markdown@2.0.7 ### Patch Changes - Updated dependencies \[]: - @khanacademy/perseus-utils@2.0.4 - @khanacademy/simple-markdown@2.0.7 ## @khanacademy/simple-markdown@2.0.7 ### Patch Changes - Updated dependencies \[]: - @khanacademy/perseus-utils@2.0.4 ## perseus-build-settings@0.8.0 ### Minor Changes - [#2431](#2431) [`218eb4cb1`](218eb4c) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Add CSS Modules for styling, and a converter for Aphrodite code ## @khanacademy/perseus-dev-ui@5.5.0 ### Minor Changes - [#2542](#2542) [`0729817a7`](0729817) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Overhaul of ServerItemRendererWithDebugUI to better mock actual implementation ### Patch Changes - Updated dependencies \[]: - @khanacademy/kas@2.0.7 - @khanacademy/kmath@2.0.16 - @khanacademy/math-input@26.0.5 - @khanacademy/perseus-core@14.0.3 - @khanacademy/perseus-linter@4.0.3 - @khanacademy/pure-markdown@2.0.7 - @khanacademy/simple-markdown@2.0.7
## Summary: When we landed #2431 (specifically when we moved from `rollup-plugin-styles` to `rollup-plugin-postcss` [here](https://github.com/Khan/perseus/pull/2431/files#diff-45f213140a165c743b42e38a00d97984fcec3f841195e8587d03a861d402ca17R151)), we lost some behaviour that rewrote `url()`'s found in CSS so they were relative to the `dist/` output folder and copied files they pointed to also. This PR fixes the build to use the `postcss-url` plugin to restore this behaviour. Issue: "none" ## Test plan: `pnpm build` -> check out the `dist` folder for `packages/math-input` and ensure that there's an `assets/` folder with Symbola font files in it (also that the process CSS in the dist folder points to the font files). Author: jeremywiebe Reviewers: handeyeco, mark-fitzgerald, jeremywiebe Required Reviewers: Approved By: handeyeco, mark-fitzgerald Checks: ✅ 8 checks were successful Pull Request URL: #2634
Summary:
In our effort to modernize our approach to and toolset for styling, this change adds the ability to use standard CSS syntax in a
*.module.cssfile for any of our widgets and renderers. This will enable us to migrate our styling away from Aphrodite and onto a standardized format. This is accomplished via an update to our build process. The prior Rollupstylesplugin has been replaced with thepostcssplugin since the LESS files have since been converted to standard CSS (no "building" is necessary).The conversion of Aphrodite code to standard CSS is accomplished by the addition of a script that can be run against any TSX file that contains Aphrodite code. This script file uses various
@babelplugins to generate an AST parsing of the code files, and then procedurally extracts the CSS settings and reformats them into a separate CSS file. This script does the heavy-lifting of moving and reformatting the styling, but it is expected that there are nuances in the JavaScript code that will necessitate adjustments to complete the transformation. For instance, in places where style information was passed to astyleattribute, it will now need to reference the generated class name in theclassNameattribute. This script is intended to be temporary, and only last as long as needed to convert and remove Aphrodite.Issue: LEMS-3008
Test plan:
<fieldset>element in the Radio widget (line 211 in radio/base-radio.new.tsx).cssModulesTest) pulled from a CSS file (radio/radio-test.module.css)<fieldset>parent element in the DOM now includes a class called something like_css-modules-test_6k65e_1(<8-digit-hash>)perseus_<8-digit-hash>