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(typescript): export AdjustDeeplink class #246

Closed
wants to merge 1 commit into from

Conversation

huextrat
Copy link
Contributor

@huextrat huextrat commented Sep 9, 2024

AdjustDeeplink was missing on TS types

@huextrat huextrat changed the title fix(typescript): AdjustDeeplink class fix(typescript): export AdjustDeeplink class Sep 9, 2024
@uerceg
Copy link
Contributor

uerceg commented Sep 9, 2024

Hey @huextrat,

Thank you very much for the PR! We'll try to ship the patch update with this change soon. Will keep you posted.

@uerceg
Copy link
Contributor

uerceg commented Sep 9, 2024

@huextrat, one question about this issue. How did you see this error manifesting on your end? I was trying to reproduce the error in Expo app which uses TypeScript and having following in _layout.tsx worked without any issues for me:

import { Adjust, AdjustConfig, AdjustDeeplink } from "react-native-adjust";
....
const adjustConfig = new AdjustConfig("2fm9gkqubvpc", AdjustConfig.EnvironmentSandbox);
adjustConfig.setLogLevel(AdjustConfig.LogLevelVerbose);
Adjust.initSdk(adjustConfig);
Adjust.processDeeplink(new AdjustDeeplink("url"));

Indeed, AdjustDeeplink is not exported as class, but inside of my app I'm observing that things work even with AdjustDeeplink being declared as interface. 🤔

@uerceg
Copy link
Contributor

uerceg commented Sep 9, 2024

Amazing, I'll try to reproduce the issue with your example app. Thank you very much for providing the example app.

@uerceg
Copy link
Contributor

uerceg commented Sep 10, 2024

Hey @huextrat,

Sorry for similar ping, but I am still unable to reproduce this error on my end, even with your app. Is this error supposed to be printed in native logs? I'm checking adb logcat logs for Android app and everything that Console app dumps for the iOS app process and I am not seeing the error anywhere (and weirdly enough, I do see sdk_click package with source=deeplink being tracked which pretty much means that this line executes successfully).

I'm attaching everything that iOS process shows in logs when I run your example app.

logs.txt

Any advice on what I might be doing wrong and not reproducing this issue? (also, just for the record to mention that I'm not experienced at all in some serious app development in Expo)

@huextrat
Copy link
Contributor Author

Hi @uerceg, this doesn't cause any problems at runtime, it's a TypeScript problem.

If you completely remove index.d.ts, the library will still work even with TypeScript, but types will be in error.

To reproduce, I've updated the reproducer repo with a script to run typescript lint (tsc): yarn lint:typescript

When executing this script typescript should indicate an error but as I said, it's “just” a typing error, so it's not a problem for the library to work properly :) But I assume that every developer who uses this library with a TypeScript configuration will get this type error on AdjustDeeplink.

@uerceg
Copy link
Contributor

uerceg commented Sep 10, 2024

Many thanks for the enlightenment. TIL.

And I really like this last commit of yours on the reproducer repo, I'll definitely be using that approach from now on. :)

I believe I'm now well equipped to reproduce and test the fix, so hopefully patch update is gonna be there soon.

@uerceg uerceg mentioned this pull request Sep 10, 2024
@uerceg
Copy link
Contributor

uerceg commented Sep 10, 2024

Okay, replacing this one with #247. Thank you very much for all the effort.

Also, I have noticed that with your changes if one would attempt to access (to what used to be interface named AdjustDeeplink) .deeplink field inside of the deferred deep link callback, with AdjustDeeplink being just a class with constructor that's setting .deeplink member, there was also error being fired. So I renamed the interface to make a clear distinction between the AdjustDeeplink that's being injected by clients and AdjustDeferredDeeplink that's being sent back from Adjust to users.

Looking forward to hearing back from you if v5.0.1 works as expected on your end.

@uerceg uerceg closed this Sep 10, 2024
@huextrat
Copy link
Contributor Author

Thx @uerceg, it's great like that 👍

@uerceg
Copy link
Contributor

uerceg commented Sep 10, 2024

And thank you for all the work and feedback one more time, really appreciate it.

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