Skip to content
This repository has been archived by the owner on May 13, 2023. It is now read-only.

fix: correct parsing of boolean values #30

Merged
merged 2 commits into from Dec 7, 2021

Conversation

HubTreeTea
Copy link
Contributor

fix #29

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

see #29

What is the new behavior?

Correct parsing of boolean values.

Copy link
Contributor

@bdlukaa bdlukaa left a comment

Choose a reason for hiding this comment

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

Could we have some tests for this?

@HubTreeTea
Copy link
Contributor Author

Could we have some tests for this?

I guess you're talking about two more test cases in transformers_test.dart? If so, I can add them, of course.

Copy link
Contributor

@bdlukaa bdlukaa left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@dshukertjr dshukertjr left a comment

Choose a reason for hiding this comment

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

@HubTreeTea Thanks for an amazing fix 🎉🎉🎉

Looking at the js client library code here, I'm wondering if we need this chunk at all within convertColumn.

final columnValueStr = columnValue == null
      ? null
      : columnValue is String
          ? columnValue
          : columnValue.toString();

@HubTreeTea
Copy link
Contributor Author

@HubTreeTea Thanks for an amazing fix tadatadatada

Looking at the js client library code here, I'm wondering if we need this chunk at all within convertColumn.

final columnValueStr = columnValue == null
      ? null
      : columnValue is String
          ? columnValue
          : columnValue.toString();

I'm not certain what you mean @dshukertjr. I did see that the code you listed is in the repository and when I was debugging, I did not know why this .toString() call was being done (but that it resulted in the conversion of boolean values to their string counterparts and therefore the wrong parsing afterwards).
If you're thinking about whether to remove this, I'm not sure if I can make a qualitative decision here.

Is there anything left I can do?

@phamhieu
Copy link
Member

phamhieu commented Dec 7, 2021

lgtm!!! Thank you @HubTreeTea

@dshukertjr let fix that in another PR. Cos of the latest update on realtime server it will return the actual value instead of stringify version. @w3b6x9 correct me if i'm wrong. Maybe we should release a new major version to support that change.

@w3b6x9
Copy link
Member

w3b6x9 commented Dec 7, 2021

@phamhieu that's correct, Realtime RLS should return the actual boolean values. This client should maintain backward compatibility as well for community members who wish to use the old Realtime.

@rphly
Copy link

rphly commented Dec 7, 2021

@w3b6x9 Hi Wen Bo, after investigating, we also face this issue. Our app is currently in the wild and users are unable to access realtime features because of this bug. Is there anything we can do to rectify this issue without having to push an app update?

It doesn't quite make sense that a backend change that breaks client functionality should be released without any communication to users. Is there a way to "turn off" the new real-time until this fix is merged and more stable?

@w3b6x9
Copy link
Member

w3b6x9 commented Dec 7, 2021

@w3b6x9 Hi Wen Bo, after investigating, we also face this issue. Our app is currently in the wild and users are unable to access realtime features because of this bug. Is there anything we can do to rectify this issue without having to push an app update?

It doesn't quite make sense that a backend change that breaks client functionality should be released without any communication to users. Is there a way to "turn off" the new real-time until this fix is merged and more stable?

@rphly please email support@supabase.io with a link to your comment and your project ref and we can revert you back to the previous version of Realtime so you're not blocked.

Copy link
Member

@dshukertjr dshukertjr left a comment

Choose a reason for hiding this comment

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

@phamhieu Agreed! This PR LGTM then!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong Parsing of bool values
6 participants