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

Avoid erasing types and delegate to js export #39

Merged
merged 4 commits into from
Jul 27, 2023
Merged

Avoid erasing types and delegate to js export #39

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
377c6fe
Avoid to generate invalid kotlin code
MarcoSignoretto Jul 18, 2023
f508ddd
removed deprecated flag
MarcoSignoretto Jul 18, 2023
7a21fb6
removed deprecated flag
MarcoSignoretto Jul 18, 2023
dc2a2a2
Erase type if kustom generic is specified
MarcoSignoretto Jul 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/src/main/kotlin/deezer/kustomexport/compiler/js/mapping/TypeMapping.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class OriginTypeName(
val exportedTypeName: TypeName by lazy {
// Remove TypeParameter: because we can't export generic in a cool manner yet, so we produce concrete from generics.
// See @KustomExportGenerics
exportedType().removeTypeParameter()
exportedType()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for your work! Regarding the comments, I'm afraid that this change implies regression for types with generics. Maybe it was due to a previous Kotlin version and kt 1.7 or 1.8 doesn't require it anymore. Do you mind to investigate a bit further to check if we could have regressions?

Full disclosure, I don't use this tool anymore and don't have access to the original project to check if this is fine or not, so eventually I'll trust your judgement, just let me know ;)

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 will check. @glureau I saw also that some unit tests are failing, do you mind if I setup a workflow on GitHub Actions to run tests on PRs? for open source repo it should be free.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@glureau also I need the approval to run the PR workflow you have: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC I stopped running unit tests and instead only used the typescripts one (kind of integration tests), as I believe they have much more values.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like grgit dependency is failing the github CI. Please check locally the output of this script, we'll have to fix the CI later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is failing let me have a look

> Task :compiler:compileKotlin
w: /Users/MarcoSignoretto/Documents/careem/KustomExport/compiler/src/main/kotlin/deezer/kustomexport/compiler/GenericsVisitor.kt: (48, 28): Unchecked cast: Any? to List<KSAnnotation>
w: /Users/MarcoSignoretto/Documents/careem/KustomExport/compiler/src/main/kotlin/deezer/kustomexport/compiler/KspExt.kt: (52, 72): Unchecked cast: Any? to T
w: /Users/MarcoSignoretto/Documents/careem/KustomExport/compiler/src/main/kotlin/deezer/kustomexport/compiler/js/MethodNameDisambiguation.kt: (36, 68): 'capitalize(): String' is deprecated. Use replaceFirstChar instead.
w: /Users/MarcoSignoretto/Documents/careem/KustomExport/compiler/src/main/kotlin/deezer/kustomexport/compiler/js/Structs.kt: (42, 5): Expected performance impact from inlining is insignificant. Inlining works best for functions with parameters of functional types
w: /Users/MarcoSignoretto/Documents/careem/KustomExport/compiler/src/main/kotlin/deezer/kustomexport/compiler/js/mapping/CustomMapping.kt: (184, 42): Parameter 'typeName' is never used, could be renamed to _
w: /Users/MarcoSignoretto/Documents/careem/KustomExport/compiler/src/main/kotlin/deezer/kustomexport/compiler/js/pattern/FunctionTransformer.kt: (31, 53): 'capitalize(): String' is deprecated. Use replaceFirstChar instead.
w: /Users/MarcoSignoretto/Documents/careem/KustomExport/compiler/src/main/kotlin/deezer/kustomexport/compiler/js/pattern/SealedClassTransformer.kt: (42, 9): Variable 'ctorParams' is never used

> Task :samples:compileKotlinJs FAILED
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsConsumer.kt: (23, 55): No type arguments expected for external interface GenericsInterface
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsConsumer.kt: (30, 61): No type arguments expected for external interface GenericsInterfaceFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsConsumer.kt: (37, 50): No type arguments expected for class GenericsImpl
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsConsumer.kt: (44, 56): No type arguments expected for class GenericsImplFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsFactory.kt: (22, 57): No type arguments expected for class GenericsImpl
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsFactory.kt: (29, 62): No type arguments expected for class GenericsImplFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterface.kt: (19, 34): No type arguments expected for external interface GenericsStuff
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterface.kt: (25, 60): No type arguments expected for external interface GenericsStuff
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterface.kt: (26, 35): No type arguments expected for external interface GenericsStuff
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterface.kt: (58, 53): No type arguments expected for external interface GenericsStuff
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterface.kt: (77, 30): Type of 'bar' doesn't match the type of the overridden var-property 'public abstract var bar: [Error type: Type for error type constructor (GenericsStuff)]<Double>? defined in sample.generics.js.GenericsInterface'
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterface.kt: (77, 43): No type arguments expected for external interface GenericsStuff
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterface.kt: (97, 69): No type arguments expected for external interface GenericsStuff
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterface.kt: (98, 35): No type arguments expected for external interface GenericsStuff
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterfaceFloat.kt: (21, 39): No type arguments expected for external interface GenericsStuffFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterfaceFloat.kt: (27, 65): No type arguments expected for external interface GenericsStuffFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterfaceFloat.kt: (28, 40): No type arguments expected for external interface GenericsStuffFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterfaceFloat.kt: (59, 58): No type arguments expected for external interface GenericsStuffFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterfaceFloat.kt: (78, 30): Type of 'bar' doesn't match the type of the overridden var-property 'public abstract var bar: [Error type: Type for error type constructor (GenericsStuffFloat)]<Float>? defined in sample.generics.js.GenericsInterfaceFloat'
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterfaceFloat.kt: (78, 48): No type arguments expected for external interface GenericsStuffFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterfaceFloat.kt: (98, 74): No type arguments expected for external interface GenericsStuffFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/GenericsInterfaceFloat.kt: (99, 40): No type arguments expected for external interface GenericsStuffFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (21, 79): No type arguments expected for external interface GenericsInterfaceFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (32, 26): Unresolved reference: bar
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (34, 22): Unresolved reference: bar
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (38, 31): Unresolved reference: fooBar
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (45, 31): Unresolved reference: fooBars
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (53, 31): Unresolved reference: addListener
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (54, 58): No type arguments expected for external interface GenericsStuffFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (65, 31): Unresolved reference: baz
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (76, 12): 'bar' overrides nothing
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (76, 48): No type arguments expected for external interface GenericsStuffFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (82, 12): 'fooBar' overrides nothing
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (89, 12): 'fooBars' overrides nothing
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (96, 12): 'addListener' overrides nothing
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (96, 74): No type arguments expected for external interface GenericsStuffFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (97, 40): No type arguments expected for external interface GenericsStuffFloat
e: /Users/MarcoSignoretto/Documents/careem/KustomExport/samples/build/generated/ksp/js/jsMain/kotlin/sample/generics/js/SuperGenericsInterfaceFloat.kt: (109, 12): 'baz' overrides nothing

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':samples:compileKotlinJs'.
> A failure occurred while executing org.jetbrains.kotlin.compilerRunner.GradleCompilerRunnerWithWorkers$GradleKotlinCompilerWorkAction
   > Compilation error. See log for more details

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 6s

}

fun exportedMethod(name: FormatString) = exportMethod(name)
Expand Down
52 changes: 52 additions & 0 deletions compiler/src/test/kotlin/deezer/kustomexport/compiler/TypeMappingTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,58 @@ class TypeMappingTest {
)
}

@Test
fun maps() {
assertCompilationOutput(
"""
package foo.bar
import deezer.kustomexport.KustomExport

@KustomExport
class ClassWithMaps {
var myMap: Map<String, Int>
}
""",
ExpectedOutputFile(
path = "foo/bar/js/ClassWithMaps.kt",
content = """
package foo.bar.js

import kotlin.Int
import kotlin.String
import kotlin.Suppress
import kotlin.collections.Map
import kotlin.js.JsExport
import foo.bar.ClassWithMaps as CommonClassWithMaps

@JsExport
public class ClassWithMaps() {
internal var common: CommonClassWithMaps

init {
common = CommonClassWithMaps()
}

public var myMap: Map<String, Int>
Copy link
Contributor Author

@MarcoSignoretto MarcoSignoretto Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

before this was like public var myMap: Map

get() = common.myMap
set(setValue) {
common.myMap = setValue
}

@Suppress("UNNECESSARY_SAFE_CALL")
internal constructor(common: CommonClassWithMaps) : this() {
this.common = common
}
}

public fun CommonClassWithMaps.exportClassWithMaps(): ClassWithMaps = ClassWithMaps(this)

public fun ClassWithMaps.importClassWithMaps(): CommonClassWithMaps = this.common
""".trimIndent()
)
)
}

@Test
fun from_Long_to_Double() {
assertCompilationOutput(
Expand Down
2 changes: 0 additions & 2 deletions gradle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
kotlin.mpp.stability.nowarn=true
kotlin.mpp.enableGranularSourceSetsMetadata=true
kotlin.native.enableDependencyPropagation=false

kotlinVersion=1.7.22
kspVersion=1.7.22-1.0.8