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

fix(deps): set compileOnly dependencies #56

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Conversation

matrei
Copy link
Contributor

@matrei matrei commented Oct 16, 2024

The jakarta-servlet-api was previously included as a transitive dependency, but it is no longer available through that path. To resolve the issue, this commit explicitly adds jakarta-servlet-api as a compileOnly dependency to ensure the necessary classes are available during compilation without bundling them into the final artifact.

Also change scopes of groovy and grails-core dependencies to compileOnly as these will always be provided to a grails plugin.

The `jakarta-servlet-api` was previously included as a transitive dependency,
but it is no longer available through that path. To resolve the issue,
this commit explicitly adds `jakarta-servlet-api` as a `compileOnly` dependency
to ensure the necessary classes are available during compilation without bundling them into the final artifact.

Also change scopes of `groovy` and `grails-core` dependencies to `compileOnly` as these will always be provided to a grails plugin.
@matrei matrei requested a review from codeconsole October 16, 2024 05:21
@codeconsole
Copy link
Contributor

codeconsole commented Oct 16, 2024

@matrei yeah, unfortunately now the plugins have the burden of resolving the servlet-api. I don't think there is a way to make it transitive, but compile only?

Due to AST transformations. Any plugin that has a TagLib or Controller will need the servlet-api added in order to be able to compile it.

@codeconsole
Copy link
Contributor

Quick question before you merge, should we be resolving the servlet-api and other versions from the bom instead?

@@ -5,6 +5,7 @@ greenmail = '2.0.1'
groovy = '4.0.23'
gsp = '7.0.0-SNAPSHOT'
javamail = '2.0.1'
servlet-api = '6.0.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be taken from the bom?

Copy link
Contributor

@codeconsole codeconsole Oct 16, 2024

Choose a reason for hiding this comment

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

It doesn't look like we have the servlet-api in the bom, but perhaps it should be there? Just because it is in the bom, doesn't me it will be resolved, but at least the correct version will be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the other versions, could be resolved from the bom? gsp, groovy, etc?

@matrei
Copy link
Contributor Author

matrei commented Oct 16, 2024

I don't think there is a way to make it transitive, but compile only?

Isn't that what the compileOnlyApi scope is for?

@codeconsole
Copy link
Contributor

I don't think there is a way to make it transitive, but compile only?

Isn't that what the compileOnlyApi scope is for?

that's why I was asking. yeah, that looks like the best path..

The goal is to minimize dependency resolution and dependency imports. We have way too many dependency imports in our starter app that should not be needed.

We really need to create a list of plugins that use AST transforms and identify what imports they need specific to those transforms. Those would probably be the only location for compileOnlyApi.

@codeconsole
Copy link
Contributor

Either way the servlet-api is going to be needed for

https://github.com/gpc/grails-mail/blob/5.0.x/src/main/groovy/grails/plugins/mail/MailMessageContentRenderer.groovy

And there are no ast transforms or traits in this class that will propagate the resolution of the servlet-api

@codeconsole codeconsole merged commit da501c8 into 5.0.x Oct 16, 2024
4 checks passed
@matrei matrei deleted the matrei/add-servlet-api branch October 17, 2024 06:45
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.

2 participants