Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Issue #142 #149

Merged
merged 1 commit into from
Nov 20, 2024
Merged

Issue #142 #149

merged 1 commit into from
Nov 20, 2024

Conversation

vazarkevych
Copy link
Collaborator

No description provided.

Comment on lines +90 to +95
private lateinit var errorMessage: String

/**
* Error Stacktrace for the caught error / exception
*/
val stackTrace: String? = error?.stackTraceToString()
private lateinit var stackTrace: String

Choose a reason for hiding this comment

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

Hello! Why are these variables private again?
I am using this to log why GrowthBook might have failed to load, but now it's all hidden.
It was recently opened up in #138, can we make them public again? Or make the throwable itself public?

Copy link

@yuvalgnessin-qz yuvalgnessin-qz Nov 25, 2024

Choose a reason for hiding this comment

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

Adding to this, would it be possible for breaking changes like this to be indicated via a release note and/or by bumping the major or minor version name? 1.1.62 -> 1.1.63 is a patch version update that does not really communicate that there is a breaking API change in the SDK.

Thank you very much!

Choose a reason for hiding this comment

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

Adding to this, would it be possible for breaking changes like this to be indicated via a release note and/or by bumping the major or minor version name? 1.1.62 -> 1.1.63 is a patch version update that does not really communicate that there is a breaking API change in the SDK.

Thank you very much!

I agree that these breaking changes shouldn't be done in a patch release. We're working on streamlining the release process.

We'll do a fix to make these params in GBError accessible. We're sorry for any inconvenience caused. Thanks!


/**
* The assigned value of the feature
*/
val value: Any?,
val value: JsonElement?,
Copy link

@albertoastiazaran-qz albertoastiazaran-qz Nov 25, 2024

Choose a reason for hiding this comment

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

Changing this from Primitive -> JsonElement is also a pretty big API change. Can we please document this change somewhere? It's not mentioned anywhere in the changelog for v1.1.63.

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

written just now here

@@ -23,8 +23,7 @@ import com.sdk.growthbook.utils.toHashMap
import kotlinx.coroutines.flow.Flow
import kotlinx.serialization.json.JsonObject

typealias GBTrackingCallback = (GBExperiment, GBExperimentResult) -> Unit
typealias GBExperimentRunCallback = (GBExperiment, GBExperimentResult) -> Unit
typealias GBTrackingCallback = (GBExperiment, GBExperimentResult?) -> Unit
Copy link

Choose a reason for hiding this comment

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

Hello! We noticed that GBExperimentResult became nullable.
In which cases can null arrive and why? Because also is not mentioned in changelog for v1.1.63

Copy link
Collaborator

Choose a reason for hiding this comment

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

In order to answer the question: null can arrive in result of remote evaluation.
It is good that you have noticed this change. It seems like in JS SDK the second parameter is not nullable.
The pull request to rollback was created:
https://github.com/growthbook/growthbook-kotlin/pull/154/files

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants