feat(computed-artifact): support arbitrarily many inputs #2705
Conversation
| * Asserts that the length of the array is the same as the number of inputs the class expects | ||
| * @param {!Array<*>} artifacts | ||
| */ | ||
| _assertCorrectNumberOfArtifacts(artifacts) { |
There was a problem hiding this comment.
Arity testing feels like overkill here. It seems like any error would be caught as arity errors usually are in JS (the derived class's compute_() would throw). It also rules out optional arguments, which I believe the cache setup in this PR should handle transparently (absent this check)
There was a problem hiding this comment.
optional arguments won't work because it relies on artifacts array being the same length for every call
consider
artifacts.requestMyComputed(1)
artifacts.requestMyComputed(1, 2)we get a cannot read property has of undefined error because the cache map was created expecting a nesting level of 1 but needed 2, it seems like a reasonable sanity check and keeps the internal structure simple. if a computed artifact wants to have something as optional it can support passing in undefined as the n-th argument instead, but it'll have to be explicitly passed for now
There was a problem hiding this comment.
ah, yeah, we'd have to move to storing a value and a Map per node, which isn't so bad. IMO requiredNumberOfArtifacts is a code smell (this is a cache, not a type system), but since we don't have any audits that even want to use multiple arguments yet, I'm ok with waiting for a user if you don't want to implement now.
| throw new Error(`${className} requires ${expected} artifacts but ${actual} were given`); | ||
| } | ||
| } | ||
|
|
||
| /* eslint-enable no-unused-vars */ | ||
|
|
||
| /** | ||
| * Request a computed artifact, caching the result on the input artifact. | ||
| * @param {*} artifact |
There was a problem hiding this comment.
update jsdoc ("on the input artifacts.", @param {...*} artifacts)
|
|
||
| return computedArtifact.request(obj0, obj1) | ||
| .then(result => assert.equal(result, 0)) | ||
| .then(() => assert.deepEqual(computedArtifact.lastArguments, [obj0, obj1, mockComputed])) |
There was a problem hiding this comment.
does testing lastArguments add more information? It seems like testing result is sufficient to check that a cached result is being returned.
There was a problem hiding this comment.
nothing has been testing that compute actually received the proper arguments, this is a pretty critical thing considering leaving the original line untouched (aside from the variable rename) would have totally borked every computed artifact
There was a problem hiding this comment.
nothing has been testing that compute actually received the proper arguments
well, the computed artifact subclass tests that don't take shortcuts are :)
Anyway, that's fine.
| const multiInputArtifact = new MultipleInputArtifact(); | ||
|
|
||
| return Promise.resolve() | ||
| .then(() => singleInputArtifact.request(1)) |
There was a problem hiding this comment.
the file is already using _ for non-mocha ignorable parameters, so should probably match that
There was a problem hiding this comment.
while we're on this topic, why is that style a lot of places haha? is it the extra character?
There was a problem hiding this comment.
while we're on this topic, why is that style a lot of places haha? is it the extra character?
it predates the use of eslint on lighthouse #8 :)
I think the original conversation was that it's nice to indicate that an argument is passed in but is ignored (borrowed from several other languages) and got stretched to cover even when there are no parameters (presumably because it's one fewer characters).
Mocha does mess with that a bit, though.
patrickhulce
force-pushed
the
computed_artifact_tuples
branch
from
1d83736
to
4eca276
Compare
|
@brendankenny PTAL |
fixes #2275
opens the door for #2703 to be built on computed artifacts and re-used in other places