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

Tailwind MegaSpike #6574

Closed
wants to merge 64 commits into from
Closed

Tailwind MegaSpike #6574

wants to merge 64 commits into from

Conversation

bkrmendy
Copy link
Contributor

@bkrmendy bkrmendy commented Oct 22, 2024

Problems explored on this spike

Patching removed props

During canvas interactions, we write props to the inline style even in Tailwind projects, so that the updates happen as fast as possible. However, when a prop is removed during a canvas interaction, we want to override it with a 0px (or similar) value so that the relevant Tailwind class doesn’t take over. Example: while the gap prop is being adjusted, the gap property value is coming from the inline style gap prop, but as soon as it’s removed, the gap value from the tailwind class takes over, making it look like the gap is jumping back to its original size. Setting 0px works for simple values, but for more complex props like padding, this approach broke down in a surprising way: React doesn’t like it when the longhand and the shorthand version of the same CSS prop are set at the same time. This manifested in the paddings randomly disappearing. This is a well-documented wontfix in React, see facebook/react#8689. We need to tackle this problem in way that won't infect all other code around it.

The solution to this is patchRemovedProperties (and everything else it calls). While internally it's still a pretty heuristic way to solve the problem, it isolates it in a way that

  • only runStyleUpdateMidInteraction is involved in figuring out which properties need to be zeroed
  • only the strategy dispatch in involved in zeroing these props, in an explicit, opt-in way (the opt-in happens through calling patchRemovedProperties)

Code that has reading/writing from the style prop baked in

Utopia has reading/writing from the inline style prop baked in throughout the editor code, we should investigate how widespread this is and how this could be dealt with.

This lead to the following explorations:

  • I refactored AdjustCssLengthProperties to use styleInfoFactory instead of reading from element props (see the diff for details), to showcase how we can refactor a command from reading directly from the inline style prop to a style-prop agnostic approach. This refactor also showed that the StyleInfo system doesn't express the state that a prop is set, but it's a value that we can't consume (for example if instead of a CSS number, it's a JS expression or identifier), so I made the necessary adjustments.
  • The SET_PROP/UNSET_PROP actions write into the inline style prop too. I updated these actions to use the commands under the hood, so if we wanted to add some extra logic related to setting/unsetting props, the actions get those automatically.
  • There are some functions that write the inline style prop, and they are so high up the call stack that it would be very laborious to pass them down all the info the plugins might need (concretely, the Tailwind config for the Tailwind plugin). To deal with this, I added a Jotai atom + store to store the Tailwind config, and make it accessible without massive amounts of refactoring

Normalising props removed by an action or strategy / Alternative approaches to normalization

Commands (and the Utopia code in general) could modify elements that aren’t apparent from the command descriptor object. This is problematic because the normalisation step needs to know which elements to normalize, but this has to be communicated by the commands somehow. Since the commands and prop setting in general have to be audited anyway (because of the above), an alternative to the normalization approach (which we have now) could be investigated.

One way to approach this is that in a command/action that might remove a prop, we make a note somewhere that a prop has been removed, and then later down the line remove that prop from wherever it needs to be removed (inline style prop or className). However, instead of "making a note", what about removing the prop straight away? This is underpinned by the fact that most commands already use applyValuesAtPath to set props, and deleteValuesAtPath to remove props. I updated the StylePlugin type with the equivalents of applyValuesAtPath and deleteValuesAtPath, turning them into an official API to be implemented by the style plugins. Then, these functions can be called instead of writing the prop directly into the inline style.

The question of hard-to-refactor, high-up-the-call-stack functions comes up here too. We can take advantage of the fact that if a function reads/writes from/to element props (either via the props themselves or via ElementInstanceMetadata), because the className prop that Tailwind needs is also among the element props (this would be a harder nut to crack if we wanted to support styled-components). Style plugins already parse element styles from props, so by creating another entrypoint for reading and writing, and we use Jotai store-based approach to get the right plugin to use, this becomes a tractable problem.

Copy link
Contributor

github-actions bot commented Oct 22, 2024

Try me

Copy link

relativeci bot commented Oct 22, 2024

#15089 Bundle Size — 58.07MiB (+0.01%).

0f4b889(current) vs d1d46c1 master#15082(baseline)

Warning

Bundle contains 70 duplicate packages – View duplicate packages

Bundle metrics  Change 3 changes Regression 1 regression
                 Current
#15089
     Baseline
#15082
Regression  Initial JS 41.05MiB(+0.01%) 41.04MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 21.47% 18%
No change  Chunks 20 20
No change  Assets 22 22
Change  Modules 4168(+0.02%) 4167
No change  Duplicate Modules 213 213
No change  Duplicate Code 27.3% 27.3%
No change  Packages 477 477
No change  Duplicate Packages 70 70
Bundle size by type  Change 2 changes Regression 1 regression Improvement 1 improvement
                 Current
#15089
     Baseline
#15082
Regression  JS 58.06MiB (+0.01%) 58.06MiB
Improvement  HTML 7.37KiB (-0.25%) 7.39KiB

Bundle analysis reportBranch spike/plugin-prop-setterProject dashboard


Generated by RelativeCIDocumentationReport issue

@bkrmendy bkrmendy force-pushed the spike/plugin-prop-setter branch from 5a27aca to 8b1bf1f Compare October 22, 2024 10:43
@@ -75,45 +67,45 @@ export function setCssLengthProperty(
}
}

export const runSetCssLengthProperty: CommandFunction<SetCssLengthProperty> = (
export const runSetCssLengthProperty = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

runSetCssLengthProperty is refactored not to read directly from the inline style prop

Comment on lines 193 to 207
export function runStyleUpdateForStrategy(
commandLifecycle: InteractionLifecycle,
editorState: EditorState,
elementPath: ElementPath,
updates: StyleUpdate[],
) {
switch (commandLifecycle) {
case 'mid-interaction':
return runStyleUpdateMidInteraction(editorState, elementPath, updates)
case 'end-interaction':
return getActivePlugin(editorState).updateStyles(editorState, elementPath, updates)
default:
assertNever(commandLifecycle)
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

writing the inline styles instead of the whatever the active plugins needs during an interaction

@bkrmendy bkrmendy changed the title Spike/plugin prop setter Tailwind MegaSpike Nov 6, 2024
@bkrmendy bkrmendy mentioned this pull request Nov 6, 2024
2 tasks
bkrmendy added a commit that referenced this pull request Nov 11, 2024
See #6574 for a long-form
writeup on the full background of this PR.

## The problem addressed on this PR
The normalisation step cannot easily handle commands/actions removing
properties from elements.

## Fix
Instead of working around the limitation of the normalisation step, this
PR introduces an approach where the style plugins themselves implement
setting/removing style props on elements, and the `SetProperty` /
`DeleteProperty` command uses the style plugin API to set/remove style
props. The PR deals with patching props that are removed during a canvas
interaction (details in
#6574, TLDR: if `gap` is
removed with the gap control, it needs to be set to `0px` while the
interaction is in progress, otherwise the value from the Tailwind class
will be applied which would be janky).

This is an under-the-hood PR, no user-facing functionality is added.

### Details
- Updated the `StylePlugin` interface with the `updateStyles` function,
which can be used to set/delete element styles. The inline style plugin
and the Tailwind style plugin are updated accordingly
- Injected `StylePlugin.updateStyles` into `runSetProperty` and
`runDeleteProperties` via the new `runStyleUpdateForStrategy` function.
This function encapsulates a performance-related feature: if an
interaction is ongoing, it sets props in the inline style prop (and
patches removed props), so updates are real-time, and when an
interaction is done, it uses the active plugin. This trick works because
the active strategy is re-run on interaction finish, producing the right
patches for a style update.
- The machinery dealing with zeroing props is added. The entrypoint for
adding the zeroed props is `patchRemovedProperties` (called from
`dispatch-strategies.tsx`, and the entrypoint for recording which props
need to be zeroed is `runStyleUpdateMidInteraction`
- The code related to the normalisation step is removed
- `UpdateClassListCommand` is removed and turned into a helper function
(since it's not called as a command anymore, just as a helper in the
Tailwind style plugin)

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Play mode
bkrmendy added a commit that referenced this pull request Nov 20, 2024
## Problem
`useInspectorInfo`, a widely used helper in the inspector, cannot read
element styles set by Tailwind

## Fix
The fix is made up from multiple pieces:
- I extended `StylePlugin` with a new function,
`readStyleFromElementProps`, which reads a given style info key from the
`JSXAttributes` passed to it. I updated both `InlineStylePlugin` and
`TailwindStylePlugin` to support this function, and updated all affected
tests. This change was originally prototyped on the
[Megaspike](#6574)
- I updated `useGetMultiselectedProps` to use
`StylePlugin.readStyleFromElementProps` when a style prop is read.
- I updated `CSSStylePropertyNotParsable` and `ParsedCSSStyleProperty`
to preserve the original value read from `projectContents`, in addition
to the parsed representation. This way, `CSSStyleProperties` can be used
by code that expect to work with this lower-level representation (such
as the internals of `useInspectorInfo`)
- I updated the `SET_PROP` and `UNSET_PROP` actions to use the
`setProperty` and `deleteProperty` commands under the hood. This way,
any editor code using these actions will be able to use the style
plugins to write element style. This change was originally prototyped on
the [Megaspike](#6574)

### Out of scope
This change only touches `useGetMultiselectedProps` from the internals
of `useInspectorInfo`. `useGetSpiedProps` is left unchanged, since the
values element style props set through Tailwind don't show up in
`allElementProps` (which is the data source for spied props)

## Manual Tests:
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Play mode
liady pushed a commit that referenced this pull request Dec 13, 2024
See #6574 for a long-form
writeup on the full background of this PR.

## The problem addressed on this PR
The normalisation step cannot easily handle commands/actions removing
properties from elements.

## Fix
Instead of working around the limitation of the normalisation step, this
PR introduces an approach where the style plugins themselves implement
setting/removing style props on elements, and the `SetProperty` /
`DeleteProperty` command uses the style plugin API to set/remove style
props. The PR deals with patching props that are removed during a canvas
interaction (details in
#6574, TLDR: if `gap` is
removed with the gap control, it needs to be set to `0px` while the
interaction is in progress, otherwise the value from the Tailwind class
will be applied which would be janky).

This is an under-the-hood PR, no user-facing functionality is added.

### Details
- Updated the `StylePlugin` interface with the `updateStyles` function,
which can be used to set/delete element styles. The inline style plugin
and the Tailwind style plugin are updated accordingly
- Injected `StylePlugin.updateStyles` into `runSetProperty` and
`runDeleteProperties` via the new `runStyleUpdateForStrategy` function.
This function encapsulates a performance-related feature: if an
interaction is ongoing, it sets props in the inline style prop (and
patches removed props), so updates are real-time, and when an
interaction is done, it uses the active plugin. This trick works because
the active strategy is re-run on interaction finish, producing the right
patches for a style update.
- The machinery dealing with zeroing props is added. The entrypoint for
adding the zeroed props is `patchRemovedProperties` (called from
`dispatch-strategies.tsx`, and the entrypoint for recording which props
need to be zeroed is `runStyleUpdateMidInteraction`
- The code related to the normalisation step is removed
- `UpdateClassListCommand` is removed and turned into a helper function
(since it's not called as a command anymore, just as a helper in the
Tailwind style plugin)

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Play mode
liady pushed a commit that referenced this pull request Dec 13, 2024
## Problem
`useInspectorInfo`, a widely used helper in the inspector, cannot read
element styles set by Tailwind

## Fix
The fix is made up from multiple pieces:
- I extended `StylePlugin` with a new function,
`readStyleFromElementProps`, which reads a given style info key from the
`JSXAttributes` passed to it. I updated both `InlineStylePlugin` and
`TailwindStylePlugin` to support this function, and updated all affected
tests. This change was originally prototyped on the
[Megaspike](#6574)
- I updated `useGetMultiselectedProps` to use
`StylePlugin.readStyleFromElementProps` when a style prop is read.
- I updated `CSSStylePropertyNotParsable` and `ParsedCSSStyleProperty`
to preserve the original value read from `projectContents`, in addition
to the parsed representation. This way, `CSSStyleProperties` can be used
by code that expect to work with this lower-level representation (such
as the internals of `useInspectorInfo`)
- I updated the `SET_PROP` and `UNSET_PROP` actions to use the
`setProperty` and `deleteProperty` commands under the hood. This way,
any editor code using these actions will be able to use the style
plugins to write element style. This change was originally prototyped on
the [Megaspike](#6574)

### Out of scope
This change only touches `useGetMultiselectedProps` from the internals
of `useInspectorInfo`. `useGetSpiedProps` is left unchanged, since the
values element style props set through Tailwind don't show up in
`allElementProps` (which is the data source for spied props)

## Manual Tests:
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Play mode
@bkrmendy bkrmendy closed this Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant