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

feat: add generate_local_modules_build_files flag to yarn_install and npm_install rules #2449

Conversation

mistic
Copy link
Contributor

@mistic mistic commented Feb 5, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

The current expected behaviour was the one introduced by #2330
However there was a regression and after reanalysing it I believe we will be better served by hide that feature behind a flag other than try to proceed with it in an auto discoverable logic.

What is the new behavior?

With that new behaviour if you set generate_local_modules_build_files=False in both yarn_install or npm_install rules, BUILD files for symlinked node_modules will be also symlinked instead of auto generated.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mistic
Copy link
Contributor Author

mistic commented Feb 6, 2021

@alexeagle do you think we can get this fix in for the next minor?

@alexeagle
Copy link
Collaborator

It depends if it's merged before then 😊
It seems kinda hacky so maybe will need some discussion during review

@mistic
Copy link
Contributor Author

mistic commented Feb 7, 2021

👍 I've kept the previous feature logic, just removed the current check to automatically determine if the feature should run (as it is not currently working as expected) and just used a variable to control it from the rule instead

@mistic
Copy link
Contributor Author

mistic commented Feb 10, 2021

Solved the conflicts with the upstream

@mattem mattem merged commit a6449b7 into bazel-contrib:stable Feb 18, 2021
When using a monorepo it's common to have modules that we want to use locally and
publish to an external package repository. This can be achieved using a `js_library` rule
with a `package_name` attribute defined inside the local package `BUILD` file. However,
if the project relies on the local package dependency with `file:` (npm) or `link:` (yarn) to be used outside Bazel, this
Copy link

Choose a reason for hiding this comment

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

npm also supports link: and yarn also supports file:, so i'm not sure why these parentheticals are here?

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

Successfully merging this pull request may close these issues.

4 participants