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

fix: for Unable to configure Custom Domain for React-Native #1043

Merged
merged 7 commits into from
Feb 24, 2025

Conversation

sunitaprajapati89
Copy link
Contributor

fix: for Unable to configure Custom Domain for React-Native

Verified

This commit was signed with the committer’s verified signature.
pbastia Pierre Bastianelli
- updated test cases in fetchSettings.test.ts and SegmentDestination.test.ts
- added new test cases in util.test.ts
@wenxi-zeng
Copy link
Contributor

wenxi-zeng commented Feb 18, 2025

unblock e2e tests

the following should unblock the e2e the tests:

  1. update the url in this block to match the changes you made (i.e. /events to v1/b, /settings to /projects/${writekey}/settings).
  2. do the same for this block

NOTE that you need provide the write key to the endpoints

best practice

however, your changes hardcoded the path/endpoints to every domain, which disables the user to fully customize the url to their own endpoints and this is a breaking change. the suggested fix would be:

  1. add a new flag in config, something like useSegmentEndpoints and default it to false. (so we don't break existing customer who use their own path/endpoints`
  2. update your code to append Segment's path/endpoints if and only if useSegmentEndpoints is set to true. otherwise, use the one user provided as is
  3. now you can revert the changes you made on the mock server (mentioned in unblock e2e tests section)
  4. add unit tests to test against useSegmentEndpoints
  5. update readme to reflect this change

update business logic to configure custom proxy, updated test cases and tested e2e test cases
Copy link
Contributor

@wenxi-zeng wenxi-zeng left a comment

Choose a reason for hiding this comment

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

LGTM. just address the only comment and update README and it's ready to go

@wenxi-zeng wenxi-zeng merged commit b6b396a into master Feb 24, 2025
7 checks passed
@wenxi-zeng wenxi-zeng deleted the proxy-fix branch February 24, 2025 21:07
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