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

refactor: remove dynamic_deps feature #1276

Merged
merged 1 commit into from
Oct 23, 2019

Conversation

alexeagle
Copy link
Collaborator

This isn't the right way to declare that a rule depends on a plugin.

  • It is too hard for users to understand what this means, how to
    configure it correctly, and to discover that their broken tool needs a
    dynamic dep.
  • It is not npm-idiomatic. The dependency graph at install time is not
    meant to reflect the actual dependencies that will be loaded at runtime.

In the rollup example, the rollup.config.js has import 'rollup-plugin-json'
so the corresponding BUILD file should have that package in the deps.

BREAKING CHANGE:

The dynamic_deps attribute of yarn_install and npm_install is removed,
in favor of declaring needed packages in the deps/data of the rule that
invokes the tool.

@alexeagle
Copy link
Collaborator Author

This is broken because of the way directories are layed out. When we run the tool we have

execroot
- build_bazel_rules_nodejs/bazel-out/k8-fastbuild/bin/packages/rollup/test/plugins/_plugins.rollup_config.js [1]
- external/npm/rollup_binary.runfiles/node_modules/rollup [2]
- external/npm/node_modules/rollup-plugin-json [3]
- node_modules -> external/npm/node_modules

The custom module loader only looks under the runfiles.

This isn't the right way to declare that a rule depends on a plugin.

- It is too hard for users to understand what this means, how to
configure it correctly, and to discover that their broken tool needs a
dynamic dep.
- It is not npm-idiomatic. The dependency graph at install time is not
meant to reflect the actual dependencies that will be loaded at runtime.

In the rollup example, the `rollup.config.js` has `import
'rollup-plugin-json'`
so the corresponding BUILD file should have that package in the deps.

BREAKING CHANGE:

The dynamic_deps attribute of yarn_install and npm_install is removed,
in favor of declaring needed packages in the deps/data of the rule that
invokes the tool.
@alexeagle alexeagle marked this pull request as ready for review October 22, 2019 21:43
Copy link
Collaborator

@gregmagolan gregmagolan left a comment

Choose a reason for hiding this comment

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

🌮

@alexeagle alexeagle merged commit b916d61 into bazel-contrib:master Oct 23, 2019
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.

3 participants