-
Notifications
You must be signed in to change notification settings - Fork 383
[atoms] support for inline styles #1406
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
base: main
Are you sure you want to change the base?
Conversation
workflow: benchmarks/perfComparison of performance test results, measured in operations per second. Larger is better.
|
workflow: benchmarks/sizeComparison of minified (terser) and compressed (brotli) size results, measured in bytes. Smaller is better.
|
52d003a to
4d0f089
Compare
| } & ((value: V) => StyleXStyles); | ||
|
|
||
| type InlineCSS = { | ||
| [Key in keyof Properties<string | number>]: InlineValue< |
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.
It looks like everything in Properties is optional and the Required utility type can help.
| [Key in keyof Properties<string | number>]: InlineValue< | |
| [Key in keyof Required<Properties<string | number>>]: InlineValue< |
It would be cool if StyleXCSSTypes.js also used csstype or at least the same thing.
nmn
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.
Good start, but a few changes are needed.
| const valueProxy = (_propName) => | ||
| new Proxy(function () {}, { | ||
| get() { | ||
| return valueProxy(''); | ||
| }, | ||
| apply() { | ||
| return undefined; | ||
| }, | ||
| }); |
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.
It should probably not be a recursive proxy and the get() should throw an error similar to the stylex package saying you need to compile this away.
| if (typeof prop === 'string') { | ||
| return valueProxy(prop); | ||
| } | ||
| return undefined; |
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.
You can probably just throw here directly and not define valueProxy at all.
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.
Also, let's add Flow types to this and auto-generate the TS types from that.
| if (specifier.type === 'ImportNamespaceSpecifier') { | ||
| state.inlineCSSImports.set(specifier.local.name, '*'); | ||
| } else if (specifier.type === 'ImportDefaultSpecifier') { | ||
| state.inlineCSSImports.set(specifier.local.name, '*'); |
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.
ESM doesn't allow an arbitrary proxy for exports from a package. So maybe we ONLY allow a named or default import here?
I think limiting this to a named import makes the most sense right now.
| import * as css from '@stylexjs/inline-css'; | ||
| stylex.props(css.display.flex); |
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.
| import * as css from '@stylexjs/inline-css'; | |
| stylex.props(css.display.flex); | |
| import {css} from '@stylexjs/inline-css'; | |
| stylex.props(css.display.flex); |
| stylex.props([{ | ||
| "color": color != null ? "x14rh7hd" : color, | ||
| "$$css": true | ||
| }, { |
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.
It is important that that the style objects created by the inline-css package are hoisted to the module level as a constant and not inlined like this. We already have a utility for doing this somewhere.
| module.exports = inlineCSS; | ||
| module.exports.default = inlineCSS; |
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 feel like it's rare at this point to see frontend code be published CJS-only. Could this be written in ESM and then use babel at build time to create a CJS version?
I have a similar nitpick about styleq but I see there is an open PR for that. One advantage is that it would make the @stylexjs/stylex rollup bundle slightly smaller.
- Rename package from inline-css to utility-styles - Update package.json, README, types, and source files - Update stylex package dependency and re-export
4d0f089 to
fc6cd07
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| expect(output).toContain('.x78zum5{display:flex}'); | ||
| expect(output).toContain('.x14rh7hd{color:var(--x-color)}'); | ||
| expect(output).toContain('--x-color'); | ||
| expect(output).toContain('_v != null ? "x14rh7hd" : _v'); |
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.
Can we stick to snapshot tests here? You can keep these checks in addition if you want to, but I think this is a bad pattern than LLMs generate.
| k1xSpc: "x78zum5", | ||
| $$css: true | ||
| }; | ||
| stylex.props(_temp, styles.opacity(0.5));" |
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.
Can you add a few more tests with this kind of pattern. I want to see what happens to inline styles when applied conditionally but stylex.props cannot be pre-compiled more.
Also, to validate the hoisting logic, can you make add some examples where the stylex.props() exists inside of a function and not the top level.
|
|
||
| import { transformSync } from '@babel/core'; | ||
| import { messages } from '../src/shared'; | ||
| import { messages } from '@stylexjs/shared'; |
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.
Did you just decide to move things back to the shared package?
| : [...external, '@dual-bundle/import-meta-resolve', '@stylexjs/stylex'], | ||
| plugins: [ | ||
| babel({ babelHelpers: 'bundled', extensions, include: ['./src/**/*'] }), | ||
| babel({ babelHelpers: 'bundled', extensions, include: ['./src/**/*', '../shared/src/**/*'] }), |
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 should not be needed. The shared package itself should be transformed so no need to run it through Babel.
(Maybe we should move the refactor work to a separate PR?)
| +stylexWhenImport: Set<string> = new Set(); | ||
| // Map of local identifier -> imported name. | ||
| // For namespace/default imports we store '*'. | ||
| +inlineCSSImports: Map<string, string> = new Map(); |
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.
let's discuss the right naming here.
| // `stylex.create` calls | ||
| +styleMap: Map<string, CompiledNamespaces> = new Map(); | ||
| +styleVars: Map<string, NodePath<>> = new Map(); | ||
| +inlineStylesCache: Map<string, FlatCompiledStyles> = new Map(); |
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.
What is this? Why is it needed?
| import * as t from '@babel/types'; | ||
| import StateManager from '../utils/state-manager'; | ||
|
|
||
| const UTILITY_STYLES_SOURCES = new Set(['@stylexjs/utility-styles']); |
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.
/style-properties?
- utility-styles/babel-transform.js: transforms css.display.flex → { display: 'flex' }
- babel-plugin/stylex-props.js: compiles raw objects using styleXCreateSet
- No dependency on @stylexjs/shared refactor (uses ../shared)
- All 797 tests pass
- Rename package from @stylexjs/utility-styles to @stylexjs/atoms - Update all imports and references in babel-plugin - Update stylex package dependency and re-export - Fix type definitions to be compatible with stylex.props() - Update test descriptions and imports
fc6cd07 to
fb5a7ae
Compare
Implementation
Adding support for inline styles authoring with a new
@stylexjs/atomspackage. This allows you to write styles inline instylex.props()that are pre-compiled within the props visitor using much of thecreatelogic for static and dynamic styles.This is something we went back and forth on a few times, but ultimately felt that it's good to provide the optionality. We still recommend (and want to bake in enough friction) for people to use
stylex.createnamespaces for the majority of usecases for readability/maintainability reasons.The naming of all the above is up for discussion, as well as what styles we should build into this API. I'm also using
Propertiestype from estree for type checking for a first pass, but there might be something better we can use.Testing
Style types:
Tested within docs page the above +
Integration work: