Skip to content
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

refactor(core): css-keyframes feature extraction #2215

Merged
merged 27 commits into from
Jan 2, 2022

Conversation

idoros
Copy link
Collaborator

@idoros idoros commented Dec 21, 2021

fix

  • unresolved imported keyframes have no diagnostics - see reproduction
  • change current nesting diagnostics that only reports under @st-scope to report under any unknown nesting with the exception of @media and stylesheet root
  • don't report @keyframes override under different @media rules (see demo) - keep report on import override or any other collision.
  • fix global keyframes with no name from registering a symbol

refactor

  • move related code into the feature (KeyframesSymbol, reservedKeyFrames)
  • processor - move code to analyzeAtRule
  • transformer - split keyframes resolve/transform/export into the general flow
    • transformResolve (new hook) - move resolveKeyframes to feature
    • transformAtRuleNode (new hook) - move to feature
    • transformDeclaration (new hook) - move animation/animation-name namespace to feature
    • transformJSExports (new hook) - move to feature
  • change st-import feature to register keyframes to st-symbol
    • make the registration plugable from css-keyframes
  • align redeclare keyframes symbol with other symbol diagnostics through st-symbol

deprecate

  • meta.keyframes
  • meta.mappedKeyframes

tests

  • move tests to features/css-keyframes.spec
  • validate feature tests completeness
    • align tests structure
    • add missing tests
      • check keyframes symbol collection
      • check local override of imported symbol
      • check diagnostics report for redeclare keyframes
      • js-mixin with st-global, unknown and known keyframes mixed-in

move to backlog

@idoros idoros added core Processing and transforming logic tech debt Updates, upgrades, stale code and work-arounds labels Dec 21, 2021
@idoros idoros self-assigned this Dec 21, 2021
- `KeyframeSymbol`
- `reservedKeyframes`
- analyze `@keyframes`
- default generic type to `createFeature` in case no specific nodes are required
- transform `@keyframes` at-rules
- transform `animation` and `ainmation-name` declarations
- export keyframes JS exports
- add new feature hooks: `transformResolve`, `transformAtRule`, `transformDeclaration`, and `transformJSExports`
- extract scope function to `helpers/namespace`
- remove transformer commented code
- remove leftover `KeyframesSymbol` that was previously copied to `css-keyframes` feature
- add test for `keframes` symbol collection
- add test for local override of imported symbol
- add `getKeyframesStatements()` to `css-keyframes` feature
- no public api for any atm since the API should account for multiple definitions nested in `@media` at-rules
- `meta.keyframes` - internally `CSSKeyframes.get()`
- `meta.mappedKeyframes` - internally `CSSKeyframes.getKeyframesStatements()`
- new internal getter for all keyframes symbols - `CSSKeyframes.getAll()`
…ture

- add static registration of typed imports by function (currently just for keyframes)
- `@keyframe st-global()` definition in mixin
- conflicted keyframes between mixin and stylesheet takes stylaesheet resolved definitions
- add ToDo for animation-name declaration from mixin symbols that cannot be resolved atm
packages/cli/src/base-generator.ts Show resolved Hide resolved
packages/core/src/features/st-symbol.ts Show resolved Hide resolved
packages/core/src/stylable-resolver.ts Outdated Show resolved Hide resolved
packages/core/src/features/css-keyframes.ts Show resolved Hide resolved
packages/core/src/features/css-keyframes.ts Outdated Show resolved Hide resolved
packages/core/test/features/st-import.spec.ts Show resolved Hide resolved
@barak007 barak007 self-requested a review December 29, 2021 14:50
leave as is for now to be solved consistently with other escaping issues
Copy link
Contributor

@tzachbon tzachbon left a comment

Choose a reason for hiding this comment

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

Looks great, left some comments

packages/core/src/helpers/rule.ts Outdated Show resolved Hide resolved
packages/core/src/features/css-keyframes.ts Show resolved Hide resolved
packages/core/src/features/css-keyframes.ts Show resolved Hide resolved
packages/core/src/features/st-import.ts Show resolved Hide resolved
packages/core/src/helpers/escape.ts Outdated Show resolved Hide resolved
packages/core/test/features/css-keyframes.spec.ts Outdated Show resolved Hide resolved
packages/core/test/features/css-keyframes.spec.ts Outdated Show resolved Hide resolved
packages/core/test/features/css-keyframes.spec.ts Outdated Show resolved Hide resolved
packages/core/test/features/css-keyframes.spec.ts Outdated Show resolved Hide resolved
- add `@supports` to `isInConditionalGroup`
- add comment for `safeRedeclare` on keyframes symbols
- add comment about `st-import` feature `ImportTypeHook` static registration
- removed `escapeIdentifier` from `helpers/escape` for now
- changed all tests to use `@st-import` instead of `:import`
- test typo
@idoros idoros merged commit ea3e67d into master Jan 2, 2022
@idoros idoros deleted the ido/keyframes-feature-extraction branch January 2, 2022 17:13
@idoros idoros mentioned this pull request Jan 19, 2022
33 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Processing and transforming logic tech debt Updates, upgrades, stale code and work-arounds
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants