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

#49 adds extension functions module #53

Merged
merged 10 commits into from
Apr 2, 2020
Merged

#49 adds extension functions module #53

merged 10 commits into from
Apr 2, 2020

Conversation

willbuck
Copy link
Contributor

No description provided.

@morki
Copy link
Contributor

morki commented Jan 18, 2020

@willbuck Nice, thank you very much :) Am i supposed to close PR #44 now?

@willbuck
Copy link
Contributor Author

@morki I think that makes sense, yeah. Thank you for already doing so, and thank you for your contribution!

@graemerocher
Copy link
Contributor

This looks like a good start but I think we need more documentation in the user guide and also I would restructure the extensions so that they are not all in io.micronaut.kotlin but instead in packages that relate to the classes they apply to. For example io.micronaut.kotlin.http.client for the http client extensions

As more extensions are built this kind of organization will become important IMO

@willbuck
Copy link
Contributor Author

That's definitely doable @graemerocher , I will get on that!

@willbuck
Copy link
Contributor Author

@damianw and @bram209 Interested in your ideas to improve this PR as well, feedback is welcome!

Comment on lines 25 to 31
implementation "com.fasterxml.jackson.module:jackson-module-kotlin:2.10.2", {
exclude group:"org.jetbrains.kotlin", module:'kotlin-reflect'
}
implementation "io.micronaut:micronaut-inject:$micronautVersion"
implementation "io.micronaut:micronaut-runtime:$micronautVersion"
implementation "io.micronaut:micronaut-http-server:$micronautVersion"
implementation "io.micronaut:micronaut-http-client:$micronautVersion"
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding this module will add all of these dependencies on the runtime classpath of applications that use them. I don't think we can safely assume users want runtime, http-server, or http-client. What can be done to only enable the extensions if the dependency is present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not super familiar with all the various gradle lifecycle configurations for dependencies; is there a alternative to implementation (and something corresponding in source?) to mark classes to be optionally loaded only if something they depend on is present? Perhaps that's possible, I'm not sure, but I don't think I've heard of such a thing.

Alternatively, I could separate the extensions for those modules into corresponding kotlin modules (like having a micronaut-runtime-kotlin-ext subproject, a micronaut-http-client-kotlin-ext subproject, etc).

Copy link
Contributor

Choose a reason for hiding this comment

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

is there a alternative to implementation (and something corresponding in source?) to mark classes to be optionally loaded only if something they depend on is present?

I think what you want is compileOnly. It will allow the code to compile but will not load these dependencies at runtime, allowing users to optionally include them on their project's dependencies. More info here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@edrd-f Yes, however I don't know how Kotlin handles extension classes for classes that aren't on the classpath. If it simply ignores them then great.

Copy link
Contributor

@edrd-f edrd-f Feb 4, 2020

Choose a reason for hiding this comment

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

@jameskleeh did a test now. It compiles and at runtime behaves just like Java: NoClassDefFoundError, which makes sense since it's not possible to get an instance of the class prior to the extension call.

@bram209
Copy link

bram209 commented Feb 4, 2020

Will try to review soon

@morki
Copy link
Contributor

morki commented Feb 23, 2020

Any progress here? :)

@morki
Copy link
Contributor

morki commented Mar 3, 2020

@jameskleeh @willbuck is there anything blocking?

@willbuck
Copy link
Contributor Author

willbuck commented Mar 3, 2020

@morki I've been assigned to working on other things lately, apologies for the late reply; I think the hangup was around making sure there was a way to not require runtime, http-server and http-client, as well as just awaiting review.

I've tried out something that might accomplish the former, but need to double check @edrd-f 's assertion that this will not damage dependent apps, and also confer w/ the team that the NoClassDefFound behavior is appropriate.

@edrd-f
Copy link
Contributor

edrd-f commented Mar 3, 2020

Hi @willbuck. Sorry for not giving more info on my previous comment.

Extension functions are compiled to static functions, receiving the instance of the extended type as first argument. So, for example:

Main.kt
fun String.removeSpaces(): String {
    return this.replace(" ", "")
}

Will produce a bytecode that when decompiled to Java looks like:

@Metadata(...)
public final class MainKt {
   @NotNull
   public static final String removeSpaces(@NotNull String $this$removeSpaces) {
      Intrinsics.checkParameterIsNotNull($this$removeSpaces, "$this$removeSpaces");
      return StringsKt.replace$default($this$removeSpaces, " ", "", false, 4, (Object)null);
   }
}

As you can see, it's a new class, not a modification of the original class' bytecode.

In this case, since the extended type is received as argument, if it's not present in the classpath at runtime, a NoClassDefFoundError will be thrown even before the extension function is called. This is the same behavior of Java in case the code depends on a type that's not defined in runtime.

@willbuck
Copy link
Contributor Author

willbuck commented Apr 2, 2020

@edrd-f @morki @jameskleeh Did my latest commit address the primary concerns around merging this new module, so we can provide these extension functions to folks?

@morki
Copy link
Contributor

morki commented Apr 2, 2020

@willbuck for me this is ok, but I didn't have any concerns, I would just love to use those functions, thank you very much for your work :)

@edrd-f
Copy link
Contributor

edrd-f commented Apr 2, 2020

Looks good to me. And by the way, great work!

@jameskleeh jameskleeh merged commit 98c20ab into master Apr 2, 2020
@jameskleeh jameskleeh deleted the issue-49 branch April 2, 2020 18:31
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

6 participants