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

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

Closed
Saadnajmi opened this issue Sep 10, 2024 · 9 comments
Closed

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

Saadnajmi opened this issue Sep 10, 2024 · 9 comments
Assignees

Comments

@Saadnajmi
Copy link

Saadnajmi commented Sep 10, 2024

Target Branch(es)

0.76

Link to commit or PR to be picked

facebook/react-native#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
Copy link
Collaborator

cc @okwasniewski

@okwasniewski
Copy link

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
Copy link
Author

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

@huntie
Copy link
Member

huntie commented Sep 13, 2024

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 😅 (facebook/react-native#39540 reverted, facebook/react-native#41808 blocked in internal CI).

@cortinico cortinico self-assigned this Sep 16, 2024
@cortinico cortinico closed this as completed by moving to Done / Picked in React Native 0.76 Releases Sep 16, 2024
@cortinico
Copy link
Collaborator

Picked, will land in 0.76.0 RC1 ✅

@cipolleschi
Copy link
Collaborator

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

@cipolleschi cipolleschi reopened this Sep 16, 2024
@cortinico
Copy link
Collaborator

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

Yup my bad, I was copy pasting too aggressively 😅

@blakef
Copy link
Collaborator

blakef commented Sep 23, 2024

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

@cipolleschi
Copy link
Collaborator

@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.

@blakef blakef closed this as completed by moving to Done / Picked in React Native 0.76 Releases Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done / Picked
Development

No branches or pull requests

6 participants