Skip to content

feat(builder-bob): dont use npm bin to get tsc path #435

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

Merged
merged 3 commits into from
Aug 25, 2023

Conversation

atlj
Copy link
Collaborator

@atlj atlj commented Aug 5, 2023

Summary

Closes #434
The builder bob typescript build phase uses yarn bin and npm bin commands to locate the tsc. However, the npm bin command is removed with npm 9.0. Please see: https://github.blog/changelog/2022-10-24-npm-v9-0-0-released/

Test plan

Both of the test cases start with the following steps:

  1. Clone the repo and cd into packages/react-native-builder-bob
  2. Run yarn prepack to build the builder bob.
  3. Run npm link
  4. Create a new project with npx create-react-native-library@latest my-awesome-project. You can select javascript as the type.
  5. Run npm link react-native-builder-bob to link the project.

Resolve tsc automatically

  1. Run npm run prepack to verify the typescript compilation step works properly.
  2. Run yarn prepack to verify the typescript compilation step works properly.
  3. Run pnpm prepack to verify the typescript compilation step works properly.

Resolve tsc using the builder bob config

  1. Go to the package.json file of the project you've generated.
  2. Go to the react-native-builder-bob section and specify the tsc binary as following: (the tsc line needs to be added.)
  "react-native-builder-bob": {
    "source": "src",
    "output": "lib",
    "targets": [
      "commonjs",
      "module",
      [
        "typescript",
        {
          "project": "tsconfig.build.json",
          "tsc": "./node_modules/.bin/tsc"
        }
      ]
    ]
  }
  1. Run npm run prepack to verify the typescript compilation step works properly.
  2. Run yarn prepack to verify the typescript compilation step works properly.
  3. Run pnpm prepack to verify the typescript compilation step works properly.

Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

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

I think it's important to ensure that we're running the local version of tsc. Using a global version of tsc isn't desired and may produce in inconsistent output. That's why there was a warning previously.

Using npx like this is problematic as it will try to install the package if it's not installed (the tsc package isn't even the official one), or run the global version if it's not present locally. I'm also not sure how it'll work in yarn monorepo (or regular projects using yarn).

The test plan also mentions that tsc option needs to be specified in bob's config - so I'm not sure if npx is working here as specifying the config skips the logic that uses npx.

This change needs to keep the previous behavior:

  • Make sure that it executes locally installed tsc bindary
  • If it's not installed locally, either use global version with a warning, or throw an error
  • Make sure that this change works in repos using yarn, pnpm, etc.,

@atlj atlj changed the title feat: use npx to run tsc feat: dont use npm bin to get tsc path Aug 6, 2023
@atlj atlj changed the title feat: dont use npm bin to get tsc path feat(builder-bob): dont use npm bin to get tsc path Aug 6, 2023
@atlj
Copy link
Collaborator Author

atlj commented Aug 6, 2023

I have removed the npx logic and now just using a bare path to determine where tsc is.
This is due to two reasons:

  1. npm bin is removed and it is not aware of workspaces
  2. npm binaries are always linked under node_modules/.bin. Please see this.

The test plan also mentions that tsc option needs to be specified in bob's config - so I'm not sure if npx is working here as specifying the config skips the logic that uses npx.

Make sure that this change works in repos using yarn, pnpm, etc.,

I've updated the testing section to support multiple cases

@atlj atlj requested a review from satya164 August 6, 2023 09:52
@satya164 satya164 merged commit ef5f6db into main Aug 25, 2023
@satya164 satya164 deleted the @atlj/replace-npm-bin-with-npx branch August 25, 2023 13:21
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.

Failed to locate 'tsc' in workspace with npm / pnpm
2 participants