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.

Sign up for GitHub

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

Jump to bottom

Remove @aspect_rules_rollup from user workspace when not needed #56

Closed
dgp1130 opened this issue Feb 14, 2023 · 1 comment
Closed

Remove @aspect_rules_rollup from user workspace when not needed #56

dgp1130 opened this issue Feb 14, 2023 · 1 comment
Labels
cleanup Internal cleanup of infrastructure

Comments

@dgp1130
Copy link
Owner

dgp1130 commented Feb 14, 2023 •
edited
Loading

Currently we have to include @aspect_rules_rollup for all @rules_prerender users because the build rules are referenced, even if the targets are never built because they are using prerender_pages_unbundled(). This is an unnecessary dependency and should be removed by refactoring the BUILD rules to not require when not needed.

rules_prerender/repositories.bzl

Lines 18 to 20 in 975929f

if "rollup" not in native.existing_rules():
rules_rollup_dependencies()
rollup_repositories(name = "rollup")

We also currently require users install @rollup/plugin-node-resolve in their package.json, which definitely shouldn't be required either. See #48 (comment) for more context.
@dgp1130 dgp1130 added the cleanup Internal cleanup of infrastructure label Feb 14, 2023
@dgp1130 dgp1130 mentioned this issue Feb 14, 2023
Closed
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Copies Rollup config to bin in @rules_prerender repository and link…
2e8c193 
…s it directly.

Refs #56.

By keeping the config in the `@rules_prerender` workspace, it can import the `@rollup/plugin-node-resolve` dependency at `@rules_prerender//:node_modules/@rollup/plugin-node-resolve` instead of `@//:node_modules/@rollup/plugin-node-resolve`. This removes the need for the user's workspace to have a dependency on that plugin.
dgp1130 added a commit that referenced this issue Feb 27, 2023
@dgp1130
Removes @aspect_rules_rollup dependency.
f32b3d5 
Refs #56.

In #26 we updated the Rollup integration to use a custom rule which directly invokes the `rollup` binary, so there's no need for `@aspect_rules_rollup` anymore.
@dgp1130
Copy link
Owner Author

dgp1130 commented Feb 27, 2023

In #26 I ended up deleting @aspect_rules_rollup usage since it was more convenient to make a custom rule which invokes the rollup binary directly. This has the side effect of not requiring users to manually install @aspect_rules_rollup. I was also previously able to remove @rollup/plugin-node-resolve from user-space, so we should have a proper dependency now, one which doesn't leak into user code.

@dgp1130 dgp1130 closed this as completed Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cleanup Internal cleanup of infrastructure
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@dgp1130