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

Put all of serde behind feature flag #360

Merged
merged 4 commits into from
Dec 21, 2022

Conversation

chinedufn
Copy link
Contributor

This commit makes the "serde" dependency completely optional.

Related: #357

Copy link
Contributor Author

@chinedufn chinedufn left a comment

Choose a reason for hiding this comment

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

Happy to make any changes that you want to see to get this merged.

Thank you!

Cargo.toml Outdated
@@ -61,7 +61,7 @@ default = ["grid"]
browserslist = ["browserslist-rs"]
cli = ["clap", "serde_json", "browserslist", "jemallocator"]
grid = []
serde = ["smallvec/serde", "cssparser/serde"]
with-serde = ["serde", "smallvec/serde", "cssparser/serde"]
Copy link
Contributor Author

@chinedufn chinedufn Dec 13, 2022

Choose a reason for hiding this comment

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

This is a breaking change.

We could make it non-breaking by enabling the with-serde feature by default and having a serde = [] feature, but that would enable small-vec/serde and cssparser/serde, which are not currently enabled by default.
Not sure whether or not that is an issue.

It looks like lightningcss is on an alpha train.
Is a small breaking change here acceptable?

Unless serde is commonly used by lightningcss' users, I think it makes sense to have it be disabled by default.


(Using the feature flag name "serde" as serde = ["serde", "smallvec/serde", "cssparser/serde"] wasn't possible since you can't define a feature flag with the same name as a dep.)

Copy link
Member

Choose a reason for hiding this comment

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

I was able to rename it back to serde by doing this:

serde = ["dep:serde", "smallvec/serde", "cssparser/serde"]

@devongovett
Copy link
Member

We might need to split this into two features. At the moment, this will break the Node bindings which depend on the currently not feature flagged serde derived traits. The currently flagged ones are for additional serialization support, e.g. if you wanted to serialize the whole AST.

If you run cargo build --package lightningcss_node you can see the errors. I'd rather not enable all of serde for the node binding, just the parts you've added here which are needed so we can serialize errors and dependencies that get returned. Not sure what to call the two features though, haha.

@chinedufn
Copy link
Contributor Author

Got it thanks for explaining.

I timed builds with and without all of the derives enabled and enabling them all added quite a bit to build time (15+ seconds for me).

So, makes sense.


I've added a nodejs = ["serde"] flag. The following derives are enabled when the "nodejs" feature is enabled.

  • BundleErrorKind
  • CssModuleReference
  • CssModuleExport
  • Dependency
  • ImportDependency
  • UrlDependency
  • SourceRange
  • Location
  • Error
  • ErrorLocation
  • ParseError
  • SelectorError
  • MinifyErrorKind
  • PrinterErrorKind
  • Browsers
  • CowArcStr

All other serde derives are only enabled when the "with-serde" feature is enabled.

The lighningcss_node crate has the "nodejs" feature on lightningcss enabled by default.

@chinedufn
Copy link
Contributor Author

Anything I can do to help this land so that it doesn't collect merge conflicts?

This commit makes the "serde" dependency completely optional.

Related: parcel-bundler#357
Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Thanks!

@devongovett devongovett merged commit ca3e406 into parcel-bundler:master Dec 21, 2022
@chinedufn chinedufn deleted the feature-flag-serde branch December 21, 2022 19:27
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.

None yet

2 participants