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

WIP: Add ability to parse a Style struct from a CSS string #393

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

nicoburns
Copy link
Collaborator

@nicoburns nicoburns commented Mar 11, 2023

Objective

Given how closely Taffy hews to CSS style properties, it seems like an obvious feature allow the Style struct to be parsed directly from a CSS string. Combined with #155, this would allow for a two-way serialization/deserialization to and from a much more human-friendly format than the existing serde feature. It could also be useful for exposing Taffy over FFI without the boilerplate of exposing a style builder for each language. This could be particularly natural in dynamic languages like JavaScript where stringly typed APIs may be idiomatic.

Tasks

  • Import dioxus code and make it work with Taffy 0.3
  • Add support for additional (grid and alignment) styles from Taffy 0.3
  • Sort out error handling
  • Finalise API
    • Should have both parse and update methods?
    • Should be able to parse individual styles?
  • Add to CI
  • Remove css feature from default features

Context

This code is adapted from this code in Dioxus. It uses Parcel's lightningcss as a CSS parsing library, which uses the MPL licensed cssparser crate. This seems fine to me as it doesn't require us our users to relicense our code.

Feedback wanted

What are people's thought on how best to handle errors? The current code sometimes panics when it encounters unknown/invalid styles, and sometime ignores them without feedback. There are 3 options we could pick:

  1. Panic on invalid style
  2. Silently ignore invalid styles
  3. Return errors/warnings back to caller

Personally, I'm inclined to think we ought to offer both options 2 and 3 (as separate functions) but never panic. For further context, there are technically two error cases here:

  • Styles that are genuinely invalid CSS
  • Styles that are valid CSS but we don't support

Not sure if we need to make a distinction here, but it might be useful for diagnostic purposes?

@nicoburns
Copy link
Collaborator Author

CI is complaining about duplicate dependencies. I've submitted a PR upstream to lightningcss which ought to resolve this.

@nicoburns nicoburns mentioned this pull request Mar 12, 2023
8 tasks
@Weibye
Copy link
Collaborator

Weibye commented Mar 12, 2023

Feedback wanted

What are people's thought on how best to handle errors? The current code sometimes panics when it encounters unknown/invalid styles, and sometime ignores them without feedback. There are 3 options we could pick:

  1. Panic on invalid style
  2. Silently ignore invalid styles
  3. Return errors/warnings back to caller

Personally, I'm inclined to think we ought to offer both options 2 and 3 (as separate functions) but never panic. For further context, there are technically two error cases here:

  • Styles that are genuinely invalid CSS
  • Styles that are valid CSS but we don't support

Not sure if we need to make a distinction here, but it might be useful for diagnostic purposes?

I agree that we should avoid panicking here. I'd be fine with panicking if it only were limited to the "startup of the application" but we have no such control, and we'd want to be able to have the user parse new CSS whenever they want.

I'd prefer that we give some feedback to the user whenever possible about not supported or erroneous styles but I'm not fully certain of how much complexity that would be to implement or how to best deliver this feedback to the user.

  • Styles that are genuinely invalid CSS
  • Styles that are valid CSS but we don't support

Thinking about it further, adding the above checks might be a good form of self-documentation of both the capabilities of Taffy and of current state of flexbox / grid specification. It could be a good thing to pursue regardless of the CSS parsing feature.

@RichiCoder1
Copy link

Return errors/warnings back to caller

This, though agreed there is the complexity about how to best surface this to API consumers and therefore users. I would say there is an expectation with CSS that it it's fault tolerant and best effort, so I'd expect even terribly invalid CSS to "pass" even if it's effectively non-functional. Ideally there's best effort errors however!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants