javascript: Do not read none mapped package.json files in javascript rules#18523
Conversation
|
@stuhood Not sure if the issue warrants this change, but I think something like this is what is required to fix the "only read mapped package.json's", while avoiding the rule dependency cycle. |
There was a problem hiding this comment.
Thanks a lot: this looks solid. As to @rule count: it does matter slightly, in that if something has exactly one consumer, it generally makes sense to either:
- inline it into the consumer
- use an async function (formerly known as
@rule_helper) instead, which is treated as a plain function call rather than a memoization boundary / trampoline.
But I think that if you make the suggested edit, this change will be good to go.
Sorry for the long delay on review: my only excuse is that your comment suggested that you weren't sure that it should land, but having reviewed it, it looks very reasonable.
| @rule | ||
| async def _target_generator_overrides( | ||
| req: _TargetGeneratorOverridesRequest, unmatched_build_file_globs: UnmatchedBuildFileGlobs | ||
| ) -> _Overrides: |
There was a problem hiding this comment.
Because this doesn't need to be independently memoized AFAICT, rather than a @rule it should probably be a just an async function (née @rule_helper), which would avoid needing to define the argument and return type wrappers.
There was a problem hiding this comment.
I seem to (to me at least) arbitrarily hit
native_engine.IntrinsicError: Get(Paths, PathGlobs, PathGlobs(globs=('demo/f1.ext',), glob_match_error_behavior=<GlobMatchErrorBehavior.warn: 'warn'>, conjunction=<GlobExpansionConjunction.any_match: 'any_match'>, description_of_origin='the `overrides` field for demo:demo')) was not detected in your @rule body at rule compile time. Was the `Get` constructor called in a separate function, or perhaps dynamically? If so, it must be inlined into the @rule body.when switching to a rule helper (just regular async function) here?
There was a problem hiding this comment.
Is MultiGet different from Get in this instance?
There was a problem hiding this comment.
There was a problem hiding this comment.
Is this worth adding to the "probable issue" bit of the error?
This bit:
Was the `Get` constructor called in a separate function, or perhaps dynamically? If so, it must be inlined into the @rule body.?
There was a problem hiding this comment.
Actually, considering your suggestions, that error seems not correct in any sense? The Get in the helper in question is dynamic (if-clause) afaict?
There was a problem hiding this comment.
Yea, for sure: that message hasn't been updated since rule helpers became supported.
There was a problem hiding this comment.
I opened #18663, not sure about the wording but imo still better than current state 🤷♂️
| @@ -490,7 +495,28 @@ async def read_package_jsons(globs: PathGlobs) -> PackageJsonForGlobs: | |||
|
|
|||
| @rule | |||
| async def all_package_json() -> AllPackageJson: | |||
There was a problem hiding this comment.
It would be good to add a note here that explains why this avoids using the more obvious APIs.
A bit hacky, considering the validation layers are skipped, but also quite safe, as the rule requires the files to exist, by virtue of it trying to read them. This also discovered a little faulty test setups with missing BUILD files 🎉
4209064 to
0b36a0e
Compare
Consider the to-be-parsed
TargetGeneratorRequestsand read the associatedpackage.jsonfrom globs beforeGenerateNodePackageTargetsis consumed to produceGeneratedTargets. It needs to be done like this to avoid a circular rule dependency that stems fromWrappedTargetbeing used to validate addresses, and_TargetParametrizationsbeing an integral part of this validation (I think?).This is an attempt at fixing point 1. of the weirdness observed in #18326 (comment).
I also took some liberties in refactoring
resolve_target_parametrizations, because I struggled to navigate it. It does mean more@rules in thegraphmodule though, I dont know if that is a metric to be weary of?