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

Add tsconfig option to prerender_*() #21

Closed
dgp1130 opened this issue Jan 16, 2021 · 1 comment
Closed

Add tsconfig option to prerender_*() #21

dgp1130 opened this issue Jan 16, 2021 · 1 comment
Labels
bug Something isn't working
Milestone

Comments

@dgp1130
Copy link
Owner

dgp1130 commented Jan 16, 2021

Right now, all the ts_library() rules being generated do not specify a tsconfig, so I believe they default to //:tsconfig.json. This happens to work in rules_prerender because that is where the tsconfig is, but user repository may not follow that convention. We should give users a hook to provide their own tsconfig.

@dgp1130 dgp1130 added the bug Something isn't working label Jan 16, 2021
@dgp1130 dgp1130 added this to the 1.0.0 milestone Jan 16, 2021
@dgp1130
Copy link
Owner Author

dgp1130 commented Jan 16, 2021

Fixed by 6b47d6d and 7327a03 (forgot to add Fixes note to the commit message).

@dgp1130 dgp1130 closed this as completed Jan 16, 2021
dgp1130 added a commit that referenced this issue Jan 17, 2021
…d graph.

Fixes #9.

To address this problem, we split the `web_resources()` rule into two different directories:
1. A `merge` directory, which is the same as the existing one, containg all the files from all transitive resources.
2. An `entries` directory, which contains **only** the files explicitly listed as entries.

The first directory is computed from the transitive closure of the second across all dependencies. Because each `merge` directory is generated by merging all transitive `entries` directories, no erroneous duplicate files exist and the "diamond"/"triangle" pattern works as expected.

Also updated the `resources` example to serve as a test case for this dependency pattern.

We still use the same resource packager binary for both actions, even though it could (and arguably should) be done by two distinct tools. The current state of affairs isn't really a problem, so we can probably get away with leaving it until they diverge enough that splitting the tool into two would be beneficial.

One downside is that `web_resources()` rules are not validated until they `merge` directory is depended upon (by a devserver for instance). This is bad for two reasons:
1. If a `web_resources()` rule has two dependencies which conflict with each other, the target will likely still build and pass tests on its own as the `merge` directory is not directly depended upon.
2. A conflict will only fail the rule at the very top-level of the dependency graph. Meaning that a devserver or another target that depends on the `merge` directory will see that `web_resources()` rule fail, even when a transitive `web_resources()` dependency is the one that actually encounters the merge conflict.

These are both a bit awkward, but not the end of the world. If Bazel had better support for validating actions, this would be an easy fix (just make sure the `merge` directory is built before letting someone depend on the `entries` directory). However a fix for this is more annoying than necessary at the moment. Filed #21 to address this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant