-
Notifications
You must be signed in to change notification settings - Fork 276
Improvements I think we should include in Lift-4.0 #2044
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
Improvements I think we should include in Lift-4.0 #2044
Conversation
…met creation and initialization functions. * Updated dependencies to newer Jakarta and Apache versions and replaced deprecated methods. * Refactored liftContextRelativePath() to a function returning a List[String] for the Lift-path. This provides more flexibility when interacting with other frameworks.
|
@farmdawgnation Do my suggested changes look sane? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR modernizes Lift for version 4.0 by upgrading to Servlet 6.1 and Jakarta EE, refactoring the Lift path handling from a single string to a dynamic list of path segments, and enhancing comet actor creation with initialization functions. The changes have been tested in production for several months.
Key Changes
- Upgraded servlet API to 6.1 and migrated remaining
javax.*imports tojakarta.* - Refactored
LiftRules.liftContextRelativePathfromStringto() => List[String]for multi-tenant support (breaking change) - Enhanced comet actor API with optional initialization and factory functions
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web/webkit/src/test/scala-2.13/net/liftweb/http/LiftSessionSpec.scala | Updated test calls to use new comet creation signature with identity init function |
| web/webkit/src/main/scala/net/liftweb/http/provider/servlet/HTTPRequestServlet.scala | Updated import from servlet5 to servlet6 for file upload |
| web/webkit/src/main/scala/net/liftweb/http/provider/encoder/CookieEncoder.scala | Replaced deprecated length() check with nonEmpty for StringBuilder |
| web/webkit/src/main/scala/net/liftweb/http/js/LiftJavaScript.scala | Updated PageJs pattern matching to use new List-based path structure |
| web/webkit/src/main/scala/net/liftweb/http/SecurityRules.scala | Updated CSP violation handler pattern to match new List-based path structure |
| web/webkit/src/main/scala/net/liftweb/http/S.scala | Added two new comet creation methods with init function and factory function support |
| web/webkit/src/main/scala/net/liftweb/http/Req.scala | Updated standardRequest_? check to use new List-based path (contains bug) |
| web/webkit/src/main/scala/net/liftweb/http/LiftSession.scala | Enhanced comet creation with init and factory functions (contains bug in init path) |
| web/webkit/src/main/scala/net/liftweb/http/LiftServlet.scala | Updated path length checks and pattern matching for List-based paths |
| web/webkit/src/main/scala/net/liftweb/http/LiftRules.scala | Changed liftContextRelativePath to function returning List[String], added pageJsFunc |
| web/webkit/src/main/scala/net/liftweb/http/LiftMerge.scala | Refactored page JS URL generation to use new path structure |
| web/testkit/src/main/scala/net/liftweb/mocks/*.scala | Removed deprecated method, added servlet 6.1 method stubs, added override keywords |
| project/Dependencies.scala | Updated to servlet 6.1, jakarta.mail 2.1.5, and various other dependency updates |
| core/util/src/main/scala/net/liftweb/util/Mailer.scala | Migrated from javax.mail to jakarta.mail and javax.activation to jakarta.activation |
| core/util/src/test/scala/net/liftweb/util/MailerSpec.scala | Updated imports from javax.mail to jakarta.mail |
| core/util/src/main/scala/net/liftweb/util/SecurityHelpers.scala | Reorganized imports (no functional change) |
| core/util/src/main/scala/net/liftweb/util/Schedule.scala | Changed buildExecutor return type from ThreadPoolExecutor to ExecutorService |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
I don't have this part of the build in CI/CD yet, but there are compile failures for Scala 3: Will need to investigate those before merge here. |
|
Sorry, didn't answer your original question here which is I think the goal of these changes make sense to me. We just need to investigate some of the copilot comments and compile issues. :) |
|
@farmdawgnation Excellent, then it makes sense to fix these issues. If nobody was interested in having this it wouldn't make sense to put more effort into it. I'll have a look at the copilot issues. How do I build with Scala3? |
If you check it out locally, you should be able to start the sbt interactive console, then |
|
Yes, I did that but it resulted in insanely many more errors: So I figured I must be doing something wrong... |
|
@farmdawgnation I'm using Intellij IDEA and have built the code successfully there, but it builds for 2.13. Any idea how to make IDEA build for 3.3.7? |
|
Tests aren't compiling yet for anything except webkit. That's why it's not in CI yet. That's my next project. I don't use IDEA so, unfortunately, not. :( |
|
Ah, I see. So if I make the webkit-tests compile with Scala-3 we're good?
on the mailing-list, so we agree on that then we don't have to fix the failing tests in |
* Removed Mailer and BCrypt
|
This PR also removes
|
|
@farmdawgnation Please let me know if there's anything I can do to help getting this PR merged. If all good, please "squash and merge" at your convenience. |
|
@andreak It's the Thanksgiving Holiday in the US so I haven't had a chance to look at it yet. Going to try this morning but I'm herding tiny humans as well. :) |
|
✅ Tests are passing in 2.13 I think we're good here. |
|
🎸 ! |
The breaking change here is
LiftRules.liftContextRelativePath(), but I think this is important in order to make Lift highly configurable and able to interoperate with other frameworks. We use this to set tenantId for multitenant config while running as root-webapp.We've been running these suggested changes in production for several months now (based on the 3.5-branch) without any issues.