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 api kotlinx-atomicfu dependency for JS and WASM targets. #475

Merged
merged 4 commits into from
Oct 18, 2024

Conversation

mvicsokolova
Copy link
Collaborator

Fixes #448

@mvicsokolova mvicsokolova requested a review from fzhinkin October 9, 2024 14:56
@@ -50,6 +50,11 @@ kotlin {
}
}

// Workaround for KT-71203. Can be removed after https://github.com/Kotlin/kotlinx-atomicfu/issues/431
atomicfu {
transformJs = false
Copy link
Contributor

@fzhinkin fzhinkin Oct 9, 2024

Choose a reason for hiding this comment

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

Should users apply the same change to their projects to use AFU w/ this PR applied without having "clashing unique_name property" error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm preparing a fix for that rn: I'll set transformJs value to false by default

Copy link
Contributor

Choose a reason for hiding this comment

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

But if, for some reason, they have this property explicitly enabled, they will get the cryptic error message anyway?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Starting with Kotlin 2.0.20 -- yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And I'll deprecate this property with a warning

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should also print an explicit warning describing the problem to guide users toward the solution? Personally, I'm struggling to link KLIB resolver blablabla unique_name blablabla with atomicfu.transformJs :)

As of #431, I remember we agreed that JS transformation removal sounds OK, but the final decision was pending (some additional research over potential downsides was required).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Regarding #431 it's only related to JS IR, and I would do deprecation (and preliminary checks) of JS IR transformations for the next release.

transformJs only disabled/enabled legacy JS transformation and now, being still enabled by default, it just creates a new js directory, causing the problem, that you've mentioned. Removing transformJs flag and legacy transformation tasks would solve the problem 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks.

Removing transformJs flag and legacy transformation tasks would solve the problem 👀

Sure, but there are a few projects that are still using it.

Copy link
Collaborator Author

@mvicsokolova mvicsokolova Oct 15, 2024

Choose a reason for hiding this comment

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

I've prepared a PR removing legacy transformations and disabling/deprecating transformJs flag here: #478

With these changes, transformJs flag does not take any effect (specifically does not create an empty is directory which caused the above mentioned error), so for the user, who used transformJs there will just be a flag deprecation warning and the WA for this error will not be needed.

So, we can merge #478, then remove the WA for transformJs from this PR, merge it as well and then publish the 0.25.1 release.

@fzhinkin fzhinkin self-requested a review October 17, 2024 16:34
Copy link
Contributor

@fzhinkin fzhinkin left a comment

Choose a reason for hiding this comment

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

I suggest releasing the changes as 0.26.0. Otherwise, everything looks good to me.

@mvicsokolova mvicsokolova merged commit adc7c19 into develop Oct 18, 2024
2 checks passed
@mvicsokolova mvicsokolova deleted the api-js-dependency branch October 18, 2024 09:40
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