Skip to content
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

Allow to pass a blob to core fetch function #71929

Merged
merged 2 commits into from Jul 24, 2023
Merged

Allow to pass a blob to core fetch function #71929

merged 2 commits into from Jul 24, 2023

Conversation

cyriltovena
Copy link
Contributor

What is this feature?

This allows to pass a Blob and not try to parse it when using the core fetch function which is used by plugins when communicating with backend service.

Why do we need this feature?

I have feature that needs to send protobuf encoded data and this is blocking.

Who is this feature for?

Mostly for developer using fetch.

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@cyriltovena cyriltovena requested a review from a team as a code owner July 19, 2023 11:10
@cyriltovena cyriltovena requested review from ashharrison90 and JoaoSilvaGrafana and removed request for a team July 19, 2023 11:10
@@ -89,6 +90,9 @@ export const parseBody = (options: BackendSrvRequest, isAppJson: boolean) => {
if (!options.data || typeof options.data === 'string') {
return options.data;
}
if (options.data.constructor.name === 'Blob') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does options.data instanceof Blob work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No apparently jest need a polyfill but I'm not sure how to do it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshhunt should we add a polyfill for this? see jsdom/jsdom#2555 and elastic/kibana#162197

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok seems like there actually is a Blob polyfill

import 'blob-polyfill';
.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure how to use it properly.

@@ -1,3 +1,4 @@
import { Blob } from 'buffer';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok this seems to be the problem. This is buffer from the node API which we (probably) don't want to use. We want to use the browser Blob (which is available globally) or use the polyfill in Jest. With this import you created a "native" Blob in the test but then were comparing it with the node Blob imported here (as that shadowed the global one).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honestly I wasn't able to make it work

@aocenas aocenas added this to the 10.1.x milestone Jul 21, 2023
@aocenas aocenas added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Jul 21, 2023
@aocenas aocenas merged commit 5c1e8c1 into main Jul 24, 2023
19 checks passed
@aocenas aocenas deleted the feat/blob-fetch branch July 24, 2023 15:02
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants