Skip to content

Upsert flags when loading feature flags with computed errors#38

Merged
marandaneto merged 2 commits into
mainfrom
fix/feature-flags-errors
Oct 5, 2023
Merged

Upsert flags when loading feature flags with computed errors#38
marandaneto merged 2 commits into
mainfrom
fix/feature-flags-errors

Conversation

@marandaneto
Copy link
Copy Markdown
Member

@marandaneto marandaneto commented Oct 5, 2023

💡 Motivation and Context

https://github.com/PostHog/posthog-js-lite/blob/4a707fdc660785c187e0bfd99ec2df1887afcf5c/posthog-core/src/index.ts#L931
I did the wrong check.
Refactor for reading the JSON from resources instead of strings, its easier to maintain and read due to identation.

💚 How did you test it?

Unit tests

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

@marandaneto marandaneto requested a review from pauldambra October 5, 2023 12:19
Copy link
Copy Markdown
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

👍


@Test
fun `merge feature flags if no errors`() {
fun `merge feature flags if there are errors`() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤯 😊

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

cheaper than the 1 billion dollar mistake tho :D

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd find these two easier to think about if they were generated by a method. So I could see the difference is the payload and the errorsWhileComputingFlags

But it's stylistic and easy to change so non-blocking

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah, I agree, that's why I named the files this way, it's a bit more obvious but there's no syntax highlighting for JSONs represented as Strings so it's also not ideal.

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