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

Attribute any Expo-apps via setAppInfo #129

Merged
merged 5 commits into from
Apr 7, 2021

Conversation

cruzach
Copy link
Collaborator

@cruzach cruzach commented Mar 26, 2021

An app is Expo managed workflow if they're using our fork of react-native (something like https://github.com/expo/react-native/archive/sdk-40.0.1.tar.gz), and an app is bare workflow if they do not use our fork, but do have the expo dependency (there are some in the bare workflow without that dependency, so this is 100% accurate, but it should capture the vast majority of folks).

I read up on Stripe's docs and wasn't sure if we should also include Expo's partnerId- for the time being I did, but let me know if I should remove that

@@ -1,10 +1,13 @@
import React, { useEffect } from 'react';

import NativeStripeSdk from '../NativeStripeSdk';
import { isAndroid } from '../helpers';
import { isAndroid, getExpoAttribution } from '../helpers';
import type { AppInfo, ThreeDSecureConfigurationParams } from '../types';
import pjson from '../../package.json';
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't pjson import the libraries package.json (https://github.com/stripe/stripe-react-native/blob/master/package.json) file? But for expo we'd need the project's package json, no? Would that just be two more folders above '../../../../package.json'; as the library is in the node_modules/stripe-react-native folder?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, those relative paths won't work. Any idea how we can get the projetc's root package.json?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there doesn't seem to be a well-supported way of getting the root project's directory or package.json, but it might be simpler to instead just use the new optional imports feature from metro:

  • if we can require('expo'), 👍
  • otherwise, 👎

see most recent commit

Copy link
Collaborator

Choose a reason for hiding this comment

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

sounds good, thanks!

@@ -1,6 +1,13 @@
import type { StripeError } from './types';
import { Platform } from 'react-native';
let isExpoInstalled = true;
try {
require('expo');
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I've tested this on the example and even though expo is an installed dependency there, it didn't work, so I logged out the error from the require statement, and it's:

 [Error: Requiring unknown module "undefined". If you are sure the module exists, try restarting Metro. You may also want to run `yarn` or `npm install`.]

But I don't know why

Copy link
Collaborator

Choose a reason for hiding this comment

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

true.. I think it will not work this way since expo in this case is in the example dependency tree (/example/node_modeules/) I will try to figure out other solution

Copy link
Collaborator

Choose a reason for hiding this comment

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

but in a real project it should be placed at the same level - I will try it and let you know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes it works but for some reason it throws an error if expo is not installed - try catch doesn't work, but should be there some workaround for this

Copy link
Contributor

Choose a reason for hiding this comment

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

@arekkubaczkowski do you not get the above error, even if expo is installed? I did some Googling, and supposedly this error happens if you import dependencies twice: facebook/metro#152 (comment) But not sure if that is the issue in our case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's due to improper version of stripe-react-native sdk installed - https://github.com/cruzach/optional-imports/blob/main/.yalc/stripe-react-native/src/helpers.ts, there is no require('expo')

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think it's using the .yalc version- confirmed by removing the .yalc/ directory and rerunning, plus I'm still seeing the console.logs from node_modules/stripe-react-native/src/helpers.ts:

try {
  require('expo');
  console.log('yes')
} catch {
  console.log('no')
  isExpoInstalled = false;
}

results in:

[Tue Mar 30 2021 10:13:05.531]  LOG      no

Copy link
Collaborator Author

@cruzach cruzach Mar 30, 2021

Choose a reason for hiding this comment

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

maybe an issue with yalc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@arekkubaczkowski let me know if there's anything else I can do here 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

@cruzach I've run your project and it works properly so probably smth was wrong with my local project. I think that we can merge it

@thorsten-stripe
Copy link
Contributor

@arekkubaczkowski also, we need to change the appinfo approach: instead of pulling it from the package.json, we should hardcode it. E.g. if someone forks the repo, we still want the same appinfo to be sent

@thorsten-stripe
Copy link
Contributor

@arekkubaczkowski also, we need to change the appinfo approach: instead of pulling it from the package.json, we should hardcode it. E.g. if someone forks the repo, we still want the same appinfo to be sent

@arekkubaczkowski actually, scratch that, it's good for us to know if someone has forked it. So let's keep withe the current approach 👍

@arekkubaczkowski
Copy link
Collaborator

@thorsten-stripe works both in example and external projects for me.

@thorsten-stripe thorsten-stripe merged commit 9ed3917 into master Apr 7, 2021
@thorsten-stripe thorsten-stripe deleted the @cruzach/expo-attribution branch April 7, 2021 14:53
arekkubaczkowski pushed a commit that referenced this pull request Apr 22, 2021
Attribute any Expo-apps via `setAppInfo`
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.

3 participants