reactwg / react-native-releases

React Native Releases Working Group
356 stars 7 forks source link

[0.76] Revert "feat: build codegen on postinstall (#46227)" #476

Closed Saadnajmi closed 1 month ago

Saadnajmi commented 1 month ago

Target Branch(es)

0.76

Link to commit or PR to be picked

https://github.com/facebook/react-native/pull/46420

Description

Revert this commit that adds a post install script for a couple of reasons:

  1. The postinstall script causes yarn install to fail on React Native macOS, where we use Yarn 4. I'm not entirely sure why, but I probably won't debug it for the rest of the reasons.
  2. postinstall scripts (at least inside Microsoft) are viewed as a security risk. Any package in your dependency tree can get compromised, add the phase, and run arbitrary code. This has happened in the past with React Native in the past if I recall correctly. As such, we disable postinstall scripts in many of our repos (including rnx-kit and react-native-test-app).
  3. The issue this is trying to solve is to help newcomers avoid a stale cache when they switch branches in the React Native monorepo and only run yarn install. I think it would be sufficient to add some documentation somewhere that it is expected one runs yarn && yarn build to use this repo locally? That's a fairly common practice in monorepos, at least ones inside Microsoft.
cortinico commented 1 month ago

cc @okwasniewski

okwasniewski commented 1 month ago

Thanks for tagging me @cortinico

@Saadnajmi sorry for this causing you some trouble 👎🏻

postinstall scripts (at least inside Microsoft) are viewed as a security risk. Any package in your dependency tree can get compromised, add the phase, and run arbitrary code.

Yeah, that's true but the monorepo package is not released to npm, its just for internal use.

I think it would be sufficient to add some documentation somewhere that it is expected one runs yarn && yarn build to use this repo locally?

Adding this to some README file could solve this issue but we have lots of README files and this detail might be easily missed.

I'm going to also loop in @huntie to get his point of view.

Saadnajmi commented 1 month ago

Update: The failure in React Native macOS was unrelated to the use of postinstall. Rest of my points still stand.

huntie commented 1 month ago

So long as the Microsoft repos aren't blocked by this change, I don't think we need to act urgently. However I agree with @Saadnajmi in terms of keeping the development process across our repos consistent.

Worth noting that this particular use case is an optional/convenience postinstall — so won't impact around Saad's point (2). But I agree we can fix this with a README note.

The long term fix is to run Codegen from source using babel-node when in the monorepo — I've had this working + in draft for a while 😅 (https://github.com/facebook/react-native/pull/39540 reverted, https://github.com/facebook/react-native/pull/41808 blocked in internal CI).

cortinico commented 1 month ago

Picked, will land in 0.76.0 RC1 ✅

cipolleschi commented 1 month ago

Actually, we decided not to pick it, as it was not landed in main.

cortinico commented 1 month ago

Actually, we decided not to pick it, as it was not landed in main.

Yup my bad, I was copy pasting too aggressively 😅

blakef commented 1 month ago

@huntie Just to confirm, what in our developer environment is calling yarn run build in codegen now that this has been removed?

cipolleschi commented 1 month ago

@blakef in the past weeks, we landed a change that was running yarn run build right after yarn install as an additional step. We don't want to run anything as post-install hook. But nothing else in how we build the project has changed.