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(analytics): update superstruct dependency #8153

Merged
merged 1 commit into from
Nov 20, 2024
Merged

Conversation

exzos28
Copy link
Contributor

@exzos28 exzos28 commented Nov 20, 2024

Related issues

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Copy link

vercel bot commented Nov 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 7:15pm

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Hey! You might have cracked this one.

  • I agree with the move to analytics
  • the new-style struct typing looks correct

Let's see what CI thinks...

@exzos28
Copy link
Contributor Author

exzos28 commented Nov 20, 2024

Could you restart "Testing E2E Android" if necessary? It looks like the emulator did not start for some reason.

@mikehardy
Copy link
Collaborator

android CI failure was unrelated, re-running it
if the iOS runs completed I think this is good to go
Fantastic! This one has personally bothered me for a long time as I'm the one usually updating dependencies and I hated seeing it stuck. Much appreciated.

@mikehardy mikehardy changed the title fix: update superstruct and move it to the only place it's used fix(analytics): update superstruct dependency Nov 20, 2024
@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar and removed Needs Attention labels Nov 20, 2024
@mikehardy mikehardy merged commit 6db1fb4 into invertase:main Nov 20, 2024
17 of 18 checks passed
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Nov 20, 2024
@RohovDmytro
Copy link

RohovDmytro commented Nov 22, 2024

I do have an error while running the app, after resetting node_modules, yarn.lock and rebuilding the app from fresh.

 ERROR  TypeError: 0, _superstruct.define is not a function (it is undefined), js engine: hermes [Component Stack]

with

@react-native-firebase/analytics@npm:21.6.0

@mikehardy
Copy link
Collaborator

rebuilding the app from fresh

@RohovDmytro 👋 looking in to this, what exact command are you running to rebuild the app from fresh?

@mikehardy
Copy link
Collaborator

mikehardy commented Nov 22, 2024

When I last ran this, this afternoon, it was already using 21.6.0 and did not have a build failure:

https://github.com/mikehardy/rnfbdemo/blob/main/make-demo.sh

@RohovDmytro
Copy link

rebuilding the app from fresh

@RohovDmytro 👋 looking in to this, what exact command are you running to rebuild the app from fresh?

Resetting watchman cache, deleting node modules, yarn lock, running yarn install and then expo prebuild, and starting metro server with a reset config. I can provide the exact command line combo a bit later, if that would be important.

@RohovDmytro
Copy link

The app does build, but crashes in production and yields the error above during development.

@mikehardy
Copy link
Collaborator

I wonder fi you have the opposite problem of the issue that prompted the PR here, #5146
That is, perhaps some other module of yours requires the old superstruct but now we rely on the new one?
Or perhaps there is some other expo-specific difference?

The app my script builds run fine in release and debug from completely clean setups (similar to what your commands do) - that's the whole point of that repro script really - to start from completely clean, do a reproducible series of steps, and have a full react-native-firebase integration run in debug and release modes. And it's working for me 🤔

@RohovDmytro
Copy link

RohovDmytro commented Nov 22, 2024

That is, perhaps some other module of yours requires the old superstruct but now we rely on the new one?

supestruct is used only via /analytics package.

├─ @react-native-firebase/analytics@npm:21.6.0
│  └─ superstruct@npm:2.0.2 (via npm:^2.0.2)
│
└─ @react-native-firebase/analytics@npm:21.6.0 [6c1df]
   └─ superstruct@npm:2.0.2 (via npm:^2.0.2)

That was the main reason I've assumed it would be enough to quickly blame open source and get away with commenting in PR discussion :)

Through years I've accumulated many ways of resetting caches. Let me try the bigest-badast--reset, maybe that will help :)

Either way, I would provide more actionable details or fix the issue my myself. Thanks for taking a look into it, @mikehardy.

@exzos28
Copy link
Contributor Author

exzos28 commented Nov 22, 2024

@RohovDmytro Did you update @react-native-firebase/analytics and @react-native-firebase/app as well?
Maybe attach your yarn.lock, I'll take a look.

@RohovDmytro
Copy link

RohovDmytro commented Nov 22, 2024

Biggest-baddest-reset did not work. My next assumptions:

  • I am using react-native 0.75.*
  • I am using hermes
  • Old arch

@exzos28, yes. All of them.

The amount of my dependencies in the current project made me mentally unstable. Please, be careful when looking at it. Thanks.

yarn.lock.zip

@RohovDmytro
Copy link

RohovDmytro commented Nov 22, 2024

Downgrading to @21.5.0 makes the project work (event without rebuilding, wow!). And that works for me :)

Another assumption: do we have latest and greatest code under 21.6.0 tag?

I have to move on with my sprint and will stop pretending to be helpful. The final straw from me is to rename the project current repository into @react-native-firearse/*, but that's also might be as helpful as previous comments.

I will post more if I'll find more.

@exzos28
Copy link
Contributor Author

exzos28 commented Nov 22, 2024

Interesting point - I also have react-native 75 and an older architecture.
And only updating superstruct allowed to use analytics package.

@mikehardy
Copy link
Collaborator

I looked through your yarn.lock @RohovDmytro and it all looks normal, unfortunately nothing jumps out at me there
My reproductions of success are

  • the test app here with react-native 0.74.x + old architecture

"react-native": "0.74.5",

And my make-demo.sh script which is the newest everything - rn0.76.2 + new architecture

and they both work - I just can't trigger this, and we haven't had others mention it yet either, so I suspect this is local somehow but I know what you mean by all the react-native project cleaning stuff (I've had PRs merged into react-native-clean-project - it's a constant battle). So it seems there may be something to this.

Unfortunately without a reproduction I can't move further with it, but I'll be listening in case something is posted

@LouisSyfer
Copy link

Same issue with my new dev build. And with the last version of analytics.

@mikehardy
Copy link
Collaborator

@LouisSyfer please be specific, "same issue" doesn't mean much when attached to a merged PR (which is not an issue, and has lots of discussion on it, with no resolution)

@LouisSyfer
Copy link

Ok sorry @mikehardy !
I have a very similar problem as described below. The dev build failed when I want to test it with Expo Go. I just removed the last version of all firebase package I have installed (21.6.1), and reinstalled the 21.2.0 version. The build is ok now.
Sorry for the misanderstanding

@exzos28
Copy link
Contributor Author

exzos28 commented Nov 26, 2024

Is it possible to use react-native-firebase/* with Expo Go? wow

@exzos28
Copy link
Contributor Author

exzos28 commented Nov 26, 2024

@LouisSyfer If possible, please create an Issue with details and a reproducible repo and let's continue the discussion there.

@mikehardy
Copy link
Collaborator

Interesting.
In fact, it may be possible to use react-native-firebase in Expo Go with our new fallback support to firebase-js-sdk - at least on the web platform. Unsure on ios/android.
Historically there is no support for Expo Go though, as we use native modules not packed in the Expo Go app

@LouisSyfer
Copy link

You're right, it's weird 🧐
I'm not sure if you can read this :

Screen_Recording_20241126_200650_Boostarz.mp4

package json partial

@mikehardy
Copy link
Collaborator

I think you're going to need expo-dev-client and get busy with prebuild

@LouisSyfer
Copy link

Expo-dev-client is installed.
I think maybe the latest versions of react-native-firebase are not compatible with SDK 52.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛] Incompatibility of superstruct versions when using @react-native-firebase/app and @solana/web3.js.
4 participants