POC: Simplifying un-abstracting projections and setup#70157
Closed
sorenlouv wants to merge 1 commit intoelastic:masterfrom
Closed
POC: Simplifying un-abstracting projections and setup#70157sorenlouv wants to merge 1 commit intoelastic:masterfrom
sorenlouv wants to merge 1 commit intoelastic:masterfrom
Conversation
sorenlouv
commented
Jun 28, 2020
| : []; | ||
|
|
||
| const params = mergeProjection(projection, { | ||
| const params = { |
Contributor
Author
There was a problem hiding this comment.
Moving away from mergeProjection will make the code a lot more clear - especially for reviewers when things are changed/moved around
Contributor
There was a problem hiding this comment.
The reason why we have mergeProjection is two-fold:
lodash.mergemerges arrays in really weird ways- if you change a projection, its consumers do not need to update anything (usually).
The downside of spreading only means that you can miss places where the projection is being used. E.g., you'd have to manually inspect all references. Maybe that's a good thing though. Again not a defense - just want to highlight why it's there.
Contributor
Author
There was a problem hiding this comment.
if you change a projection, its consumers do not need to update anything (usually).
This makes sense. Could we achieve the same with unit tests to ensure that projections are correctly applied?
Contributor
There was a problem hiding this comment.
that depends on the nature of the change. for additive changes, no. for other changes, yes (arguably already covered by snapshot testing of queries)
sorenlouv
commented
Jun 28, 2020
| }, | ||
| }) | ||
| : filter, | ||
| filter: [...projection.body.query.bool.filter, ...serviceNameFilter], |
Contributor
Author
There was a problem hiding this comment.
This is a good example of the confusion I get from the projection:
- we have a higher order function
mergeProjectionthat merges some stuff - then we have
projection.body.query.boolwhich is spreaded ontobool(is this needed if we already havemergeProjection?) - then we have a specific part of the projection being merged with the full
boolprojection + some other stuff
What takes precendence? Is ...projection.body.query.bool really needed?
In my rewrite I propose we do away with mergeProjection because it's quite opaque in it's merge behaviour. Instead I suggest we do dead-simple spread concatenations (filter: [...projection.body.query.bool.filter, ...serviceNameFilter]), or even prop assignments (index: projection.index).
Contributor
There was a problem hiding this comment.
mergeProjection = mergePlainObject, e.g. it doesn't merge arrays. If you merge arrays, you can't remove items. Arguably the same applies for merging objects, but that feels more natural. If I speak for myself, in some cases I use explicit spreading so that the shape of the request (ie the result of mergeProjection) is more clear. (Not a argument for or against projections, just an explanation).
Contributor
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack APM API integration tests (basic).x-pack/test/apm_api_integration/basic/tests/feature_controls·ts.APM specs (basic) apm feature controls APIs can be accessed by global_all userStandard OutStack TraceKibana Pipeline / kibana-xpack-agent / X-Pack APM API integration tests (basic).x-pack/test/apm_api_integration/basic/tests/feature_controls·ts.APM specs (basic) apm feature controls APIs can be accessed by global_all userStandard OutStack TraceKibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/plugins/apm/server/lib/services.services queries fetches the service itemsStandard OutStack Traceand 1 more failures, only showing the first 3. Build metrics
To update your PR or re-run it, just comment with: |
Contributor
|
@sqren just my 2$, I like |
Contributor
Author
@dgieselaar It's just yet another thing that's opaque. For instance when |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Started with a single file where
setupis no longer passed (individual params are passed instead).