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

Restructure repository layout #67

Open
dgp1130 opened this issue Feb 14, 2023 · 3 comments
Open

Restructure repository layout #67

dgp1130 opened this issue Feb 14, 2023 · 3 comments
Labels
cleanup Internal cleanup of infrastructure
Milestone
1.0.0

Comments

@dgp1130
Copy link
Owner

dgp1130 commented Feb 14, 2023

Since starting the repository I think I have a better understanding of how it should be laid out. I propose:

  • //tools/binaries/... contains all the individual tool implementations.
    • Also includes the renderer, even thought that's technically not a standalone tool as the entry point is generated on each invocation.
  • //packages/... contains individual @rules_prerender/* packages.
    • Code currently under //common/... should also be included here, and exporting a linked npm_package() though it would be listed a private: true in the package.json.
  • //prerender/... contains all the Starlark definitions, mostly aligning with the way Aspect repos organize things. I think this will avoid pulling in unnecessary dev dependencies into user workspaces if no BUILD file load()s such a dependency.

Probably some other smaller changes to consider like the difference between an example and an e2e test, but we'll start with these changes.
@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
Updates tsconfig.json paths property to reference new location fo…
1ad5e62 
…r `@rules_prerender/declarative_shadow_dom`.

Refs #67.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Moves Jasmine into //tools/jasmine/....
eb9af65 
Refs #67.

This just better organizes the code a bit, given that there's three Starlark files here related to Jasmine.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Moves TypeScript rules into //tools/typescript/....
8a427ec 
Refs #67.

This is just for better organization and to split the implementation into multiple files.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Moves //packages/annotation_extractor/... to `//tools/binaries/anno…
3bd25e2 
…tation_extractor/...`.

Refs #67.

This move is because it is an executed tool, not a package installed by the user.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Moves //packages/css_bundler/... to `//tools/binaries/css_bundler/.…
a29176e 
…..`.

Refs #67.

This is because this is a tool executed at build time, not a package installed by the user.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Moves //packages/renderer/... to //tools/binaries/renderer/....
ded80dc 
Refs #67.

This is because the renderer is invoked as a tool, not installed as a package by the user. This is a slight misnomer as `//tools/binaries/renderer/...` does not export a `js_binary()`, however it does export a library which gets wrapped into a `js_binary()` under the hood, and treated as _effectively_ a tool, even if technically it isn't.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Updates Jasmine code references to match the new location.
c1c8f6d 
Refs #67.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Moves //packages/resource_injector/... to `//tools/binaries/resourc…
f388a27 
…e_injector/...`.

Refs #67.

This is because the resource injector is invoked as a tool at build time, not a package installed by the user.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Moves //packages/resource_packager/... to `//tools/binaries/resourc…
06bc7c3 
…e_packager/...`.

Refs #67.

This is because the resource packager is a tool invoked at build time, not a package installed by users.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Moves //packages/script_entry_generator/... to `//tools/binaries/sc…
b9935f8 
…ript_entry_generator/...`.

Refs #67.

This is because the script entry generator is a tool invoked at build time, not a package installed by the user.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Deletes //tools/internal/....
4d02668 
Refs #67.

This package existed to provide a single abstraction for both the workspace source code and the published NPM package workspace in a `@build_bazel_rules_nodejs` world. However, under `@aspect_rules_js`, there is only the source workspace. This means there's no value in this layer of indirection, and all the Starlark rules can just depend directly on their associated tools.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Adds load visibility to TypeScript Starlark code.
f32d429 
Refs #67.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Adds load visibility to Jasmine Starlark code.
8c009fa 
Refs #67.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Adds load visibility to publish tools and refactors it to match new c…
686f7c5 
…onventions.

Refs #67.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Adds load visibility to stamping tools and updates to match newer con…
71876c4 
…ventions.

Refs #67.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
Adds load visibility to //packages/rules_prerender/... Starlark code.
f91b5a7 
Refs #67.

This does not attempt to refactor to the new `defs.bzl` convention just yet.
dgp1130 added a commit that referenced this issue Feb 19, 2023
@dgp1130
More load visibility restrictions.
346e745 
Refs #67.
@dgp1130
Copy link
Owner Author

dgp1130 commented Feb 19, 2023

I managed to move all the tools from //packages/... to //tools/binaries/... and refactored the Jasmine and TypeScript Starlark utilities, so I think things are a lot better already.

I tried moving //common:collections into //packages/collections/... and linking a private @rules_prerender/collections. This is only used by the annotation extractor, and I was able to update that to consume it correctly. This allowed me to drop the relative import. However, I then tried to use the same dependency from @rules_prerender/declarative_shadow_dom, being a package exposed directly to users. npm_link_package() deps can kind of do this, though I'm not entirely sure how it's supposed to work. Also, it only works within the @rules_prerender workspace, and falls apart in the user's workspace since @rules_prerender/collections isn't linked there.

I think the correct solution is to vendor @rules_prerender/collections inside @rules_prerender/declarative_shadow_dom, but I'm not sure that's actually possible. AFAICT, npm_package() doesn't actually have any direct support for this. I wonder if NPM's new-ish bundledDependencies option could do it? I'll need to keep experimenting.

One other thing we should look at as part of this issue is restricting target dependencies as much as possible. Two other things to consider are:

  1. Restrict //visibility:public to //:__subpackages__ wherever possible.
  2. Add visibility restrictions to Starlark code. I've already started this, but we'll want to refine it as we refactor //packages/rules_prerender/....

@dgp1130
Copy link
Owner Author

dgp1130 commented Feb 20, 2023 •
edited
Loading

I played around some more with bundleDependencies and got something which kinda worked, but I'm not terribly confident it's thought through enough to be usable. I filed aspect-build/rules_js#891 to discuss a more fleshed out feature in @aspect_rules_js. I think if it was added there, this would be probably be a viable path to JS code sharing, but I'm hesitant to add it myself as bespoke infrastructure for @rules_prerender.

This was referenced Mar 17, 2023
Closed
Closed
@dgp1130 dgp1130 added this to the 1.0.0 milestone Mar 17, 2023
@dgp1130
Copy link
Owner Author

dgp1130 commented Mar 17, 2023 •
edited
Loading

Marking this as a 1.0.0 blocker for the move of Starlark rules into //prerender. I don't want to force a breaking change early on just to move the definitions. Bundling dependencies isn't a hard blocker here.

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
1.0.0
Development

No branches or pull requests
1 participant
@dgp1130