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

feat: using forked segment library in cio-rn package #219

Merged
merged 14 commits into from
Dec 1, 2023

Conversation

ami-aman
Copy link
Collaborator

@ami-aman ami-aman commented Nov 21, 2023

Refers [CDP] RN iOS: Fork & Integrate Segment React Native SDK #11653

NOTE

This pull request gets merged into main_cdp_rn and not main to avoid releases before CDP work is complete and well tested. If anyone in the squad feels not aligned with this reason then please feel free to suggest a change.

@ami-aman ami-aman requested a review from a team November 21, 2023 12:21
tsconfig.json Outdated
Comment on lines 8 to 9
"@segment/analytics-react-native": ["../@segment/analytics-react-native/src/index"],
"@segment/sovran-react-native": ["../@segment/sovran-react-native/src/index"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these needed in the tsconfig file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This tell the import libraries the path to import libraries from.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do understand that is what the paths config option in tsconfig is doing.

I do not understand the reason why we need to modify the tsconfig file.

The 2 segment modules are npm dependencies declared in package.json. Therefore, I would expect that our code should be able to import these dependencies via npm without the need to modify tsconfig file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe its probably because of the fact that they are using a separate yarn workflow that sets their tsconfig.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently, the libraries are being installed from GitHub instead of npm. As a result, the tsconfig file does not find the necessary paths defined, hence throwing the errr Cannot find module '@segment/analytics-react-native' or its corresponding type declarations. Adding the path mappings to the tsconfig file resolves this issue and allow the imports to be correctly resolved. I hope this helps.

package.json Outdated Show resolved Hide resolved
tsconfig.json Outdated
Comment on lines 8 to 9
"@segment/analytics-react-native": ["../@segment/analytics-react-native/src/index"],
"@segment/sovran-react-native": ["../@segment/sovran-react-native/src/index"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe its probably because of the fact that they are using a separate yarn workflow that sets their tsconfig.

@ami-aman
Copy link
Collaborator Author

@mrehan27 @Shahroz16 @levibostian Since this PR is approved but CI tests are failing due to some yarn issues. I tried clearing yarn cache and updating checksums along with other attempts but nothing helped. Does anyone have an idea why the PR isn't passing the tests?

@mrehan27
Copy link
Contributor

@ami-aman Not sure. But probably because we mostly use npm install locally and Github action is using yarn install --frozen-lockfile. Did you try removing node_modules and running yarn install --frozen-lockfile on your local machine to get some clue?

@levibostian
Copy link
Contributor

I think I got it fixed in the commit I just pushed.

I fixed it by modifying the package.json file to using a commit hash instead of a git branch as it was set to before.

By theory is that by using a git commit hash, the lock file (should) stay up-to-date no matter what the future holds for branch aman/fork-integrate-11653. If we used a branch name, then if you pushed new commits to that branch or deleted that branch, then yarn install would fail again.

@ami-aman ami-aman merged commit 35763e6 into main_cdp_rn Dec 1, 2023
1 of 5 checks passed
@ami-aman ami-aman deleted the cdp_fork_integrate_segSDK branch December 1, 2023 11:14
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.

4 participants