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.

Sign up for GitHub

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

Jump to bottom

move all groovy-related code from "framework" to "fastergt" #316

Merged
merged 2 commits into from
Aug 9, 2024
Merged

move all groovy-related code from "framework" to "fastergt" #316

merged 2 commits into from
Aug 9, 2024

Conversation

asolntsev
Copy link
Contributor

it allows to fully avoid Groovy dependencies for users with another templating engine (e.g. Kotlin templates).

@asolntsev asolntsev mentioned this pull request Dec 30, 2023
Draft
@xabolcs
Copy link
Collaborator

xabolcs commented Dec 30, 2023

It would be nice to reference issue #65 in the commit message. 🙏

xabolcs
xabolcs reviewed Dec 30, 2023
View reviewed changes
framework/src/play/Play.java
@@ -296,8 +294,6 @@ public synchronized void start() {
private void injectStaticFields() {
injectStaticFields(Play.classes.getAssignableClasses(PlayController.class));
injectStaticFields(Play.classes.getAssignableClasses(Job.class));
injectStaticFields(Play.classes.getAssignableClasses(FastTags.class));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just viewing the diff ... : do these lines have an alternative? Or can be implemented somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xabolcs I just checked that classes FastTags and JavaExtensions do NOT have any static fields. Also, these classes don't have children. There is nothing to inject.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Answering my own question: one could move the needed code to the plugin's onApplicationStart().
For example see what cies' did in PR #227!

@asolntsev asolntsev linked an issue Dec 30, 2023 that may be closed by this pull request
Remove Groovy dependencies from framework #65
Closed
@asolntsev asolntsev self-assigned this Dec 30, 2023
@asolntsev asolntsev requested a review from xabolcs December 30, 2023 17:08
@asolntsev asolntsev force-pushed the remove-groovy-dependency branch from f787af6 to 345ca3a Compare December 30, 2023 19:56
@asolntsev asolntsev requested a review from cies December 30, 2023 19:56
xabolcs
xabolcs approved these changes Jan 1, 2024
View reviewed changes
Copy link
Collaborator

@xabolcs xabolcs left a comment

Choose a reason for hiding this comment

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

Not tested, but the diff looks good

@asolntsev asolntsev added this to the 2.4.0 milestone Jan 1, 2024
@xabolcs
Copy link
Collaborator

xabolcs commented Jan 2, 2024

There were talk about the 40x and 50x templates in how / why they need the Groovy dependency.

If it's still valid, it would be nice to see a few words / snippet in the README, or just a link to an example test app in replay-tests. What do you think?

@cies
Copy link
Member

cies commented Jan 2, 2024 •
edited
Loading

I find it odd that framework still has GT code (the templates), while it does not have the ability to execute it. See framework/src/errors and framework/src/tags. My PR has "prepared to deal with that": as I explain in the discussion of the issue. I wanted to move all GT-code (the templates) out of framework but that needs additional work (and given that my PR was already considered too big, I'm glad I did not put in that effort).

I'm not sure how you handle errors when the fastergt lib dependency is not around. With my PR one can bring their own ErrorHandler (see the interface in my PR and the SimpleErrorHandler implementation for showing pretty errors w/o GT).

In order to test your PR, I'd move the test cases from my PR to your PR and see how they are handled. I'm afraid in your PR there is no way to serve "pretty error pages" without the fastergt lib loaded.

My end goal is to completely free framework from GT-code (template code): my PR only implements step 1/2 of that goal. I think a little GT-handling code needs to stay in framework (as I explain in the discussion of the issue), but that's fairly minimal. Also: I want to be able to script pretty error pages with "my templating solution of choice", instead of still being locked into using GT for that (and thus not being able to get rid of Groovy in production).

@asolntsev
Copy link
Contributor Author

I'm not sure how you handle errors when the fastergt lib dependency is not around.

I suggest a very simple approach.
Users need to have at least some template engine. By default, it's fastergt, and users can replace fastergt by some other dependency if they wish (like Kotlin templates in your case).

@asolntsev
Copy link
Contributor Author

I find it odd that framework still has GT code (the templates)

I understand your concern. Yes, probably we should move all these classes to "fastergt" module.
Initially, I left them in "framework" just because they might be useful for authors of other templating engines. Don't you need them in your Kotlin engine?

@asolntsev asolntsev force-pushed the remove-groovy-dependency branch from 345ca3a to f856c8f Compare January 21, 2024 23:25
@xabolcs
Copy link
Collaborator

xabolcs commented Jan 22, 2024 •
edited
Loading

Users need to have at least some template engine.

May I ask to document this new framework requirement somewhere else too? In the commit description / in the README / in a talkative Exception message?

@asolntsev
Copy link
Contributor Author

@xabolcs @cies Just in case, I think this PR is finished. I've moved other templates-related classes to "fastergt" module.

But you can continue with #227 if you wish. No preference from my side.

@asolntsev
Copy link
Contributor Author

May I ask to document this new framework requirement somewhere else too?

This is not a new requirement, actually. :)
This requirement existed in Replay from the very beginning. I just didn't realize it. :)

Yes, we could write it to README, I think.
And I'll try to improve the error message as well (in case if user has not added any templating engines).

@asolntsev asolntsev modified the milestones: 2.4.0, 2.5.0 Mar 22, 2024
cies
cies approved these changes Aug 8, 2024
View reviewed changes
Copy link
Member

@cies cies left a comment

Choose a reason for hiding this comment

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

lgtm!

@asolntsev
#65 move all groovy-related code from "framework" to "fastergt"
40e950c 
it allows to fully avoid Groovy dependencies for users with another templating engine (e.g. Kotlin templates).
@asolntsev
#65 move the rest of groovy-related code from "framework" to "fastergt"
e6a7996
@asolntsev asolntsev force-pushed the remove-groovy-dependency branch from 306f0de to e6a7996 Compare August 9, 2024 15:38
@asolntsev asolntsev merged commit 3addc41 into main Aug 9, 2024
3 checks passed
@asolntsev asolntsev deleted the remove-groovy-dependency branch August 9, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xabolcs xabolcs xabolcs approved these changes

@cies cies cies approved these changes

Assignees

@asolntsev asolntsev

Labels
dependencies
Projects
None yet
Milestone
2.5.0
Development

Successfully merging this pull request may close these issues.

Remove Groovy dependencies from framework
3 participants
@asolntsev @xabolcs @cies