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

feat(DTO): Enable codegen backend by default #3215

Merged
merged 2 commits into from
Mar 17, 2024

Conversation

provinzkraut
Copy link
Member

Enable the codegen backend for DTOs introduced in #2388 by default. We have initially hidden this behind a feature flag to see how it works out in production. It's been smooth sailing for about 6 months now so I think we can enable it by default. This gives us enough time to decide if we want to remove the regular backend entirely in 3.0.

@provinzkraut provinzkraut requested review from a team as code owners March 16, 2024 14:53
Copy link

codecov bot commented Mar 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.27%. Comparing base (bfeb5e9) to head (0617e10).
Report is 116 commits behind head on develop.

❗ Current head 0617e10 differs from pull request most recent head f7bccdb. Consider uploading reports for the commit f7bccdb to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3215      +/-   ##
===========================================
+ Coverage    98.23%   98.27%   +0.03%     
===========================================
  Files          312      322      +10     
  Lines        14121    14676     +555     
  Branches      2430     2330     -100     
===========================================
+ Hits         13872    14423     +551     
- Misses         107      112       +5     
+ Partials       142      141       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@JacobCoffee JacobCoffee left a comment

Choose a reason for hiding this comment

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

i wish we had some good metrics on usage here to see how many people are trying this out.

@cofin
Copy link
Member

cofin commented Mar 16, 2024

I agree with this approach. I've been using it for some time with no issues.

@Alc-Alc
Copy link
Contributor

Alc-Alc commented Mar 17, 2024

i wish we had some good metrics on usage here to see how many people are trying this out.

While we have two users (maintainers 🙂) using and approving this feature, I have to say I am kinda of "weary" (if this comes off as a strong word, my apologies) of this approach. I feel we should "promote" this experimental feature more. What I mean is, probably have a lot of links (backlinks?) that lead to https://docs.litestar.dev/latest/usage/dto/0-basic-use.html#enabling-the-backend.

Or even have a poll (or just a normal question) in the discord to get these two data points.

  1. How many are aware and use this
  2. How stable has it been

My reason for this comment is, I (not talking about other maintainers / members 🙂 ) do not think many are aware of this. How did I arrive at that conclusion? I have not seen a significant number of users ask about this in discord. To me that could mean either of the following

  1. Anyone referring / getting inspired / copy pasting / carefully handpicking things from full stack have been using this and had zero issues. This is the "good ending" ✅ and we can go ahead.
  2. Not many have used it and we don't have a wide spread opinion on this. This is the "bad ending" ❌ and maybe make some promotion or market this as a mandatory 3.0 alpha feature.

@peterschutt
Copy link
Contributor

i wish we had some good metrics on usage here to see how many people are trying this out.

While we have two users (maintainers 🙂) using and approving this feature, I have to say I am kinda of "weary" (if this comes off as a strong word, my apologies) of this approach. I feel we should "promote" this experimental feature more. What I mean is, probably have a lot of links (backlinks?) that lead to https://docs.litestar.dev/latest/usage/dto/0-basic-use.html#enabling-the-backend.

Or even have a poll (or just a normal question) in the discord to get these two data points.

1. How many are aware and use this

2. How stable has it been

My reason for this comment is, I (not talking about other maintainers / members 🙂 ) do not think many are aware of this. How did I arrive at that conclusion? I have not seen a significant number of users ask about this in discord. To me that could mean either of the following

1. Anyone referring / getting inspired / copy pasting / carefully handpicking things from full stack have been using this and had zero issues. This is the "good ending" ✅  and we can go ahead.

2. Not many have used it and we don't have a wide spread opinion on this. This is the "bad ending" ❌ and maybe make some promotion or market this as a mandatory 3.0 alpha feature.

I see your point @Alc-Alc but the backend is supposed to be backward compatible - so as long as we do this with the guarantee that any change in behavior should be considered a bug then this shouldn't be different to rolling out any other backward compatible optimization. Also, this PR doesn't remove the current backend, so there is a pretty easy escape hatch users can apply while we address any issues.

@provinzkraut
Copy link
Member Author

Definitely need to adjust the docs though, thanks for the reminder @Alc-Alc

@provinzkraut provinzkraut force-pushed the enable-codegen-backend-by-default branch from a40557d to 0617e10 Compare March 17, 2024 08:22
@provinzkraut provinzkraut enabled auto-merge (squash) March 17, 2024 08:30
Copy link
Contributor

@Alc-Alc Alc-Alc left a comment

Choose a reason for hiding this comment

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

I was here

@provinzkraut provinzkraut force-pushed the enable-codegen-backend-by-default branch from 0617e10 to f7bccdb Compare March 17, 2024 09:00
@provinzkraut provinzkraut merged commit 8b38c09 into develop Mar 17, 2024
17 checks passed
@provinzkraut provinzkraut deleted the enable-codegen-backend-by-default branch March 17, 2024 09:04
Copy link

sonarcloud bot commented Mar 17, 2024

Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/3215

provinzkraut added a commit that referenced this pull request Mar 17, 2024
* Enable DTO codegen backend by default
* Update docs
provinzkraut added a commit that referenced this pull request Mar 18, 2024
* Enable DTO codegen backend by default
* Update docs
provinzkraut added a commit that referenced this pull request Mar 18, 2024
* Enable DTO codegen backend by default
* Update docs
JacobCoffee pushed a commit that referenced this pull request Mar 22, 2024
* Enable DTO codegen backend by default
* Update docs
provinzkraut added a commit that referenced this pull request Mar 29, 2024
* Enable DTO codegen backend by default
* Update docs
provinzkraut added a commit that referenced this pull request Mar 29, 2024
* Enable DTO codegen backend by default
* Update docs
provinzkraut added a commit that referenced this pull request Mar 30, 2024
* Enable DTO codegen backend by default
* Update docs
JacobCoffee pushed a commit that referenced this pull request Apr 5, 2024
* Enable DTO codegen backend by default
* Update docs
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.

5 participants