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

Add js target #887

Merged
merged 7 commits into from
Nov 10, 2022
Merged

Add js target #887

merged 7 commits into from
Nov 10, 2022

Conversation

hujim
Copy link
Collaborator

@hujim hujim commented Oct 25, 2022

Rebased after the Kotlin 1.6.21 update.
Reviewing by commits should help.

  • Add JS target and artifacts.
  • Fix test names to non back-ticks for common tests can't have them for JS test support.
  • Updated to use new coroutine testing library methods.
  • Handle different results ErrorLoggingInterceptor for jsTargets in tests.
  • Add JS WorkflowIdentifierTypeNamer to handle JS class comparison.

Will clean up commits once reviewed.

@hujim hujim requested review from a team and zach-klippenstein as code owners October 25, 2022 00:04
@hujim hujim mentioned this pull request Oct 25, 2022
return mapping
}

val identifier = "${kClass.simpleName ?: kClass.hashCode()}(${mappings.size})"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: This name doesn't differentiate against a genericized Workflow (although IIRC this same problem exists with kClass.qualifiedName in the native implementations too). Should I have put a comment here to mention this limitation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely put the comment about the limitations. Where were you seeing this problem for the kotlin native code? Do you have a link I can read more about this?
Was this a suggested solution for this with kotlin-js? Or something you invented?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I run the following against the JVM, it doesn't include the generic type:

data class GenericType<T>(
  val t: T
)

val someType = GenericType<String>(t = "abc")
println(someType::class.qualifiedName) // prints "com.squareup.sandbox.GenericType"

As for this implementation, I invented this solution. This seems to work in practice since it appears Kotlin/JS uses a static class (probably stored on its prototype) to represent the KClass. Open to suggestions though!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I C what you mean now. Usuallly that is when the identifier would be overridden I believe.

@@ -0,0 +1,22 @@
package com.squareup.workflow1

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see what's different here that we are requiring to expect/actual these? Is it in a later commit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's subtle but js uses simpleName instead of qualifiedName in the other targets

@@ -198,7 +200,7 @@ internal class WorkflowInterceptorTest {
key: String,
sideEffect: suspend CoroutineScope.() -> Unit
) {
runBlocking { sideEffect() }
launch { sideEffect() }
Copy link
Contributor

Choose a reason for hiding this comment

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

where do we ensure that this gets run? Here and elsewhere you could be using runTest(UnconfinedTestDispatcher() if we want everythign to go ahead eagerly.

Copy link
Collaborator Author

@hujim hujim Oct 26, 2022

Choose a reason for hiding this comment

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

Ah I might have missed this one, but I've added advanceUntilIdle() where these are required, I should add this here as well to prevent confusion.

But I've confirmed this isn't required in this particular test as though this normally could be ran in parallel, the render()+assertion still always happens at the end of the test in the single thread since it's within the test-method-level runTest block.

Does that make sense? Though it's possible I might be reading this wrong https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-test/kotlinx.coroutines.test/run-test.html

return mapping
}

val identifier = "${kClass.simpleName ?: kClass.hashCode()}(${mappings.size})"
Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely put the comment about the limitations. Where were you seeing this problem for the kotlin native code? Do you have a link I can read more about this?
Was this a suggested solution for this with kotlin-js? Or something you invented?

@@ -93,7 +93,7 @@ class RenderWorkflowInTest {
}

@Test
fun side_effects_from_initial_rendering_in_root_workflow_are_never_started_when_scope_cancelled_before_start() {
fun `side_effects_from_initial_rendering_in_root_workflow_are_never_started_when_scope_cancelled_before_start`() {
Copy link
Contributor

Choose a reason for hiding this comment

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

So we can use backticks just not spaces? was this becasue ktlint was failing these without the backticks?

Copy link
Collaborator Author

@hujim hujim Oct 26, 2022

Choose a reason for hiding this comment

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

yes precisely, we can use backticks but no special characters when compiling to JS.
ktlint marks this as a MAX_LENGTH violation without the backticks

@@ -164,6 +164,34 @@ jobs :
with :
report_paths : '**/build/test-results/test/TEST-*.xml'

js-tests :
name : JS Tests
runs-on : macos-latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd recommend ubuntu over macos for non-iOS things. macos minutes cost 10x more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The macOS runners are actually free now for public repos, but there's a limit of 5 concurrent jobs so it's still a good idea to use them sparingly.

https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good to know, I've changed to ubuntu cause these don't need mac at all.

Comment on lines 15 to 20
actual val EXPECTED_ERRORS = listOf(
"ErrorLoggingInterceptor.logBeforeMethod threw exception:\n" +
IllegalArgumentException::class.qualifiedName.toString(),
"ErrorLoggingInterceptor.logAfterMethod threw exception:\n" +
IllegalArgumentException::class.qualifiedName.toString()
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the only platform-specific part of this class is this companion object constant, then you could also move the constant to top-level and put the rest of the implementation back in common.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed!

@hujim hujim force-pushed the add-js-target-2 branch 2 times, most recently from 297e8a4 to 4a84676 Compare November 2, 2022 22:02
@hujim
Copy link
Collaborator Author

hujim commented Nov 2, 2022

@steve-the-edwards ready for a final pass
I've merged the commits where it makes sense, but left some separate for better readability.

@@ -25,3 +25,5 @@ POM_DEVELOPER_ID=square
POM_DEVELOPER_NAME=Square, Inc.
POM_DEVELOPER_URL=https://github.com/square/
SONATYPE_STAGING_PROFILE=com.squareup

kotlin.js.compiler=ir
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a link to a support article describing why this is the best?

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://kotlinlang.org/docs/js-ir-compiler.html

Rather than directly generating JavaScript code from Kotlin source code, the Kotlin/JS IR compiler backend leverages a new approach. Kotlin source code is first transformed into a Kotlin intermediate representation (IR), which is subsequently compiled into JavaScript. For Kotlin/JS, this enables aggressive optimizations, and allows improvements on pain points that were present in the previous compiler, such as generated code size (through dead code elimination), and JavaScript and TypeScript ecosystem interoperability, to name some examples.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I should have been clearer though with the comment. I meant can you include that link as a comment in the .properties file so we have reference in the future

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

would just adding the link suffice? or do we prefer also a snippet of what this is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added a bit of comment explaining the choice and the link

Copy link
Contributor

@steve-the-edwards steve-the-edwards left a comment

Choose a reason for hiding this comment

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

LGTM!

hujim and others added 7 commits November 9, 2022 11:15
@hujim hujim merged commit a39c288 into square:main Nov 10, 2022
@hujim hujim deleted the add-js-target-2 branch November 10, 2022 00:18
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

5 participants