-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Maps] Use id-values from client-manifest to suggest layers #102788
[Maps] Use id-values from client-manifest to suggest layers #102788
Conversation
Pinging @elastic/kibana-gis (Team:Geo) |
interface UniqueMatch { | ||
config: { layerId: string; field: string }; | ||
count: number; | ||
} | ||
interface FileLayerFieldShim { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of redefining interface, how about just importing FileLayerField from ems-client, https://github.com/elastic/ems-client/blob/master/src/ems_client.ts#L150
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but it's not exposed as main type yet elastic/ems-client#71, will change when that is updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the time being, can you import the type like import { FileLayerField } from '@elastic/ems-client/ems_client';
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not quite. this is an npm dependency, and the typing dependency for the package is explicitly declared to the main file (https://github.com/elastic/ems-client/blob/master/package.json#L70). Compilation will not succeed when pointing to a "sub-module".
I'd prefer to get this addressed in ems-client first, without trying to circumvent the default import behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ML changes and LGTM ⚡ 🙏
Tested on File Upload with a CSV airports dataset that contains region and country codes and the detection worked well 👏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to import "FileLayerField" with import { FileLayerField } from '@elastic/ems-client/ems_client';
? I think it would be best to have a deep import then redefine the type.
LGTM
code review
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
History
To update your PR or re-run it, just comment with: |
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Summary
NOTE FOR ML-REVIEWERS: this should be a transparent change, and not require additional changes on your end. I did make simplifications under
ml
anddata_visualizer
, but no more should be needed.The main purpose of this PR is to:
This uses the values-attribute from the EMS manifests, which were introduced in 7.14.
This PR removes the support for
emsLayerIds
. This allowed clients to explicitly check against all the fields of a layer. The downside was that the GeoJson for those layers was requested as well. This is now no longer the case.Only joins will be suggested for fields of type
id
. (e.g.KY, CA, CO, NY
). Fields of typename
(e.g.Kentucky, California, Colorado, New York
) are no longer suggested, as they are more susceptible for spelling or punctuation errors.For maintainers