Skip to content

Migrate js-stubs from kotlin-js to kotlin-multiplatform with target js#1921

Merged
robstoll merged 4 commits intorobstoll:mainfrom
ragul-engg:1887
Mar 20, 2025
Merged

Migrate js-stubs from kotlin-js to kotlin-multiplatform with target js#1921
robstoll merged 4 commits intorobstoll:mainfrom
ragul-engg:1887

Conversation

@ragul-engg
Copy link
Collaborator

resolves #1887


I confirm that I have read the Contributor Agreements v1.0, agree to be bound on them and confirm that my contribution is compliant.

@ragul-engg ragul-engg requested a review from robstoll as a code owner March 19, 2025 17:23
@codecov
Copy link

codecov bot commented Mar 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.86%. Comparing base (4e482ed) to head (adf332a).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1921   +/-   ##
=========================================
  Coverage     91.86%   91.86%           
  Complexity      120      120           
=========================================
  Files           455      455           
  Lines          5017     5017           
  Branches        240      240           
=========================================
  Hits           4609     4609           
  Misses          361      361           
  Partials         47       47           
Flag Coverage Δ
current 91.48% <ø> (ø)
current_windows 90.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

robstoll
robstoll previously approved these changes Mar 19, 2025
@robstoll
Copy link
Owner

@ragul-engg thanks for the changes. I think js-stubs was the only project using build-logic.kotlin-js. Could you please check if we can delete gradle/build-logic/dev/src/main/kotlin/build-logic.kotlin-js.gradle.kts

@ragul-engg
Copy link
Collaborator Author

Yeah, will look into that. BTW in console there is a different warning being produced do you have any idea about this ?

> Task :js-stubs:compileKotlinJs
w: Language version 1.4 is deprecated and its support will be removed in a future version of Kotlin

@ragul-engg
Copy link
Collaborator Author

Done as you have pointed out the gradle/build-logic/dev/src/main/kotlin/build-logic.kotlin-js.gradle.kts file is not used, have removed them

@robstoll
Copy link
Owner

robstoll commented Mar 20, 2025

Yeah, will look into that. BTW in console there is a different warning being produced do you have any idea about this ?

> Task :js-stubs:compileKotlinJs
w: Language version 1.4 is deprecated and its support will be removed in a future version of Kotlin

The warning is legit as we produce kotlin 1.4 compatible code. However, normally we suppress it. I guess some setup we had in build-logic.kotlin-js is missing now.

Thinking about it a bit more, we should not delete build-logic.kotlin-js -- sorry for the wrong suggestion 🙈
Instead you should use kotlin-js in js-stubs again and make the adjustments in kotlin-js, i.e base it on the multiplatform-convention.

This rename from main to jsMain directory change is required as part of
the plugin migration from kotlin-js to kotlin-multiplatform.

More Reference at Step 2 of this doc:
https://kotlinlang.org/docs/multiplatform-compatibility-guide.html#migration-from-kotlin-js-gradle-plugin-to-kotlin-multiplatform-gradle-plugin
@ragul-engg
Copy link
Collaborator Author

I did base the build-logic.kotlin-js on multiplatform-conventions. I am still seeing the same warning. I don't think we have anything specific to suppress those compileKotlinJs warnings in kotlin-js previously. Dropping the file for reference, I just replaced it to refer the mp-conventions

plugins {
    id("build-logic.kotlin-multiplatform-conventions") // changed here
}

kotlin {
    js(IR) {
        nodejs {
        }
    }
}

@ragul-engg
Copy link
Collaborator Author

ragul-engg commented Mar 20, 2025

@robstoll I found out the issue. kotlin-js build logic was lacking the kotlin-conventions, adding that suppressed the warning.

@robstoll
Copy link
Owner

Sounds like I did something wrong, the Multiplattform convention should most likely be based on kotlin-convention as well. Would you like to check?

@ragul-engg
Copy link
Collaborator Author

looks like its just based on just gradle-conventions. should i change that to kotlin-conventions as it imports gradle-conventions anyway. I think it should be fine, what do you think?

@robstoll
Copy link
Owner

I think I missed to adjust it when I introduce kotlin-convention. Please change it, we will get a CI error if our assumption is wrong

@ragul-engg
Copy link
Collaborator Author

Done ✅, lets see whether our assumption is right

Copy link
Owner

@robstoll robstoll left a comment

Choose a reason for hiding this comment

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

Looks good 🙂👍

@robstoll robstoll merged commit 099dc08 into robstoll:main Mar 20, 2025
14 of 15 checks passed
@robstoll
Copy link
Owner

Thanks for fixing more than required 🙂

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.

migrate js-stubs from kotlin-js to mpp with target js

2 participants