Conversation
|
With this PR, does a |
thomasneirynck
left a comment
thomasneirynck
left a comment
There was a problem hiding this comment.
For symmetry, I'm fine removing the node-fetch and explicitly having the client provide it.
Otherwise, we could consider modifying the two builds (one for browser and one for server). But I think for now it's important we move the Kibana NP work forward as quickly as possible, so whatever gets us there quickest is the way to go.
Yeah. I also considered throwing if no function was provided. Maybe smarter than a warning "slap on the wrist" here. Thoughts? |
nickpeihl
left a comment
nickpeihl
left a comment
There was a problem hiding this comment.
Just a couple of nits.
This is a breaking change for ems-client. But I'm not sure if we really need to document it further? ems-client "should" only be used by Kibana and ems-landing-page. ems-landing-page is client only and should not be affected. Since it appears to be breaking Kibana, perhaps it's more of a breaking bug fix? And since we're matching the stack release, it wouldn't make sense to publish a new major version anyways.
|
wrt
I think that's fine. We don't guarantee any compatibility across minors, neither for ems-client or in Kibana even (in Kibana, we don't want to break at config or end-user experience, but no guarantees in code). This will also get bumped to 7.8, so there's no real issue for existing users. And possible bug-fixes for older branches would get patched on those (7.7.x, 7.6.x, ...) |
kindsun
force-pushed
the
remove-node-fetch-dependency
branch
from
be047c4 to
ad8fabc
Compare
3 participants
Follow up to elastic/kibana#61846. Another step to unblocking elastic/kibana#60942. This PR accomplishes the following:
node-fetchfetchFunctionprovidedFeel free to suggest any revisions you think are necessary!