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.

Sign up for GitHub

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

Jump to bottom

fix: move from event-source-polyfill to fetch-event-source #24861

Merged
merged 11 commits into from
Jun 3, 2024
Merged

fix: move from event-source-polyfill to fetch-event-source #24861

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1593991
Try to switch away from polyfill
alexef May 22, 2024
f1c280b
Update unit tests
alexef May 22, 2024
db76476
Cleanup and fix build
alexef May 22, 2024
147f11f
Fix build. Move scaffolder away from polyfill
alexef May 22, 2024
cbebad1
Add changeset
alexef May 22, 2024
79605f8
Update .changeset/empty-avocados-tease.md
alexef May 22, 2024
a159480
address PR comments
alexef May 27, 2024
b7db436
update pr comments
alexef May 28, 2024
7cc45e0
Merge branch 'master' into new-events-implementation
alexef May 28, 2024
edc1801
Update .changeset/empty-avocados-tease.md
alexef May 30, 2024
9d54ebf
Fix unittests
alexef May 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/empty-avocados-tease.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@backstage/plugin-scaffolder': minor
'@backstage/plugin-techdocs': minor
alexef marked this conversation as resolved.
Show resolved Hide resolved
---

Use fetch-event-source instead of event-source-polify in order to stop hardcoding the Authorization header and reuse the fetchApi configuration.

Check failure on line 6 in .changeset/empty-avocados-tease.md

View workflow job for this annotation

GitHub Actions / check-all-files 

[vale] reported by reviewdog 🐶
[Vale.Spelling] Did you really mean 'hardcoding'?

Raw Output:
{"message": "[Vale.Spelling] Did you really mean 'hardcoding'?", "location": {"path": ".changeset/empty-avocados-tease.md", "range": {"start": {"line": 6, "column": 72}}}, "severity": "ERROR"}
alexef marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion plugins/scaffolder/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
"@material-ui/core": "^4.12.2",
"@material-ui/icons": "^4.9.1",
"@material-ui/lab": "4.0.0-alpha.61",
"@microsoft/fetch-event-source": "^2.0.1",
"@react-hookz/web": "^24.0.0",
"@rjsf/core": "5.18.2",
"@rjsf/material-ui": "5.18.2",
Expand All @@ -78,7 +79,6 @@
"@types/react": "^16.13.1 || ^17.0.0 || ^18.0.0",
"@uiw/react-codemirror": "^4.9.3",
"classnames": "^2.2.6",
"event-source-polyfill": "^1.0.31",
"git-url-parse": "^14.0.0",
"humanize-duration": "^3.25.1",
"json-schema": "^0.4.0",
Expand Down
54 changes: 25 additions & 29 deletions plugins/scaffolder/src/api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,13 @@ import { MockFetchApi, setupRequestMockHandlers } from '@backstage/test-utils';
import { rest } from 'msw';
import { setupServer } from 'msw/node';
import { ScaffolderClient } from './api';
import { EventSourcePolyfill } from 'event-source-polyfill';
import { fetchEventSource } from '@microsoft/fetch-event-source';

const MockedEventSource = EventSourcePolyfill as jest.MockedClass<
typeof EventSourcePolyfill
jest.mock('@microsoft/fetch-event-source');
const mockFetchEventSource = fetchEventSource as jest.MockedFunction<
typeof fetchEventSource
>;

jest.mock('event-source-polyfill');

const server = setupServer();

describe('api', () => {
Expand Down Expand Up @@ -87,26 +86,23 @@ describe('api', () => {
describe('streamEvents', () => {
describe('eventsource', () => {
it('should work', async () => {
MockedEventSource.prototype.addEventListener.mockImplementation(
(type, fn) => {
if (typeof fn !== 'function') {
return;
}

if (type === 'log') {
fn({
data: '{"id":1,"taskId":"a-random-id","type":"log","createdAt":"","body":{"message":"My log message"}}',
} as any);
} else if (type === 'completion') {
fn({
data: '{"id":2,"taskId":"a-random-id","type":"completion","createdAt":"","body":{"message":"Finished!"}}',
} as any);
}
},
);

const token = 'fake-token';
identityApi.getCredentials.mockResolvedValue({ token: token });
mockFetchEventSource.mockImplementation(async (_url, options) => {
const { onopen, onmessage } = options;
await Promise.resolve();
await onopen?.({ ok: true } as Response);
await Promise.resolve();
onmessage?.({
id: '',
event: 'log',
data: '{"id":1,"taskId":"a-random-id","type":"log","createdAt":"","body":{"message":"My log message"}}',
});
await Promise.resolve();
onmessage?.({
id: '',
event: 'completion',
data: '{"id":2,"taskId":"a-random-id","type":"completion","createdAt":"","body":{"message":"Finished!"}}',
});
});

const next = jest.fn();

Expand All @@ -116,14 +112,14 @@ describe('api', () => {
.subscribe({ next, complete });
});

expect(MockedEventSource).toHaveBeenCalledWith(
expect(mockFetchEventSource).toHaveBeenCalledWith(
'http://backstage/api/v2/tasks/a-random-task-id/eventstream',
{
withCredentials: true,
headers: { Authorization: `Bearer ${token}` },
fetch: fetchApi.fetch,
onmessage: expect.any(Function),
onerror: expect.any(Function),
},
);
expect(MockedEventSource.prototype.close).toHaveBeenCalled();

expect(next).toHaveBeenCalledTimes(2);
expect(next).toHaveBeenCalledWith({
Expand Down
41 changes: 19 additions & 22 deletions plugins/scaffolder/src/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import {
} from '@backstage/plugin-scaffolder-react';

import queryString from 'qs';
import { EventSourcePolyfill } from 'event-source-polyfill';
import { fetchEventSource } from '@microsoft/fetch-event-source';

/**
* An API to interact with the scaffolder backend.
Expand Down Expand Up @@ -226,11 +226,8 @@ export class ScaffolderClient implements ScaffolderApi {
params.set('after', String(Number(after)));
}

Promise.all([
this.discoveryApi.getBaseUrl('scaffolder'),
this.identityApi?.getCredentials(),
]).then(
([baseUrl, credentials]) => {
this.discoveryApi.getBaseUrl('scaffolder').then(
baseUrl => {
const url = `${baseUrl}/v2/tasks/${encodeURIComponent(
taskId,
)}/eventstream`;
Expand All @@ -245,22 +242,22 @@ export class ScaffolderClient implements ScaffolderApi {
}
};

const eventSource = new EventSourcePolyfill(url, {
withCredentials: true,
headers: credentials?.token
? { Authorization: `Bearer ${credentials.token}` }
: {},
});
eventSource.addEventListener('log', processEvent);
eventSource.addEventListener('recovered', processEvent);
eventSource.addEventListener('cancelled', processEvent);
eventSource.addEventListener('completion', (event: any) => {
processEvent(event);
eventSource.close();
subscriber.complete();
});
eventSource.addEventListener('error', event => {
subscriber.error(event);
fetchEventSource(url, {
Copy link
Member

Choose a reason for hiding this comment

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

Does this automatically cleanup? We seem to missing the eventSource.close from before.

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 assume it does. It has an onclose hook, but no explicit call to close in the documentation: https://github.com/Azure/fetch-event-source?tab=readme-ov-file#usage

Copy link
Member

Choose a reason for hiding this comment

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

It uses a signal instead, which is the more modern API. We should implement it using that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should have the option of passing in an AbortController, and default to creating one if one is not provided? Maybe can save that for a followup though..

fetch: this.fetchApi.fetch,
onmessage(e: any) {
alexef marked this conversation as resolved.
Show resolved Hide resolved
if (e.event === 'log') {
processEvent(e);
return;
} else if (e.event === 'completion') {
processEvent(e);
subscriber.complete();
return;
}
processEvent(e);
},
onerror(err) {
subscriber.error(err);
},
});
},
error => {
Expand Down
3 changes: 0 additions & 3 deletions plugins/scaffolder/src/setupTests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@

import '@testing-library/jest-dom';

const { EventSourcePolyfill } = jest.requireMock('event-source-polyfill');
global.EventSource = EventSourcePolyfill;

// Patch jsdom to add feature used by CodeMirror
document.createRange = () => {
const range = new Range();
Expand Down
4 changes: 0 additions & 4 deletions plugins/techdocs/api-report.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { DiscoveryApi } from '@backstage/core-plugin-api';
import { Entity } from '@backstage/catalog-model';
import { EntityOwnerPickerProps } from '@backstage/plugin-catalog-react';
import { FetchApi } from '@backstage/core-plugin-api';
import { IdentityApi } from '@backstage/core-plugin-api';
import { JSX as JSX_2 } from 'react';
import { PropsWithChildren } from 'react';
import { default as React_2 } from 'react';
Expand Down Expand Up @@ -465,7 +464,6 @@ export class TechDocsStorageClient implements TechDocsStorageApi_2 {
constructor(options: {
configApi: Config;
discoveryApi: DiscoveryApi;
identityApi: IdentityApi;
Copy link
Member

Choose a reason for hiding this comment

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

Thinking that we should mark this as @deprecated and make it optional and just skip using it rather than breaking change. It would only affect people who provide a custom factory to the app for creating a techdocsApiRef but it's still technically breaking.

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'm not 100% sure the way I marked it as deprecated is fine (couldn't find a similar example). Can you check?

Copy link
Member

Choose a reason for hiding this comment

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

It needs to use a doc comment, i.e. /** @deprecated ... */. Let's keep the IdentityApi type as well, instead of any

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 did the comment, but somehow I can't make it show up in the generated api-report.md.

any hints are welcome

fetchApi: FetchApi;
});
// (undocumented)
Expand All @@ -485,8 +483,6 @@ export class TechDocsStorageClient implements TechDocsStorageApi_2 {
getEntityDocs(entityId: CompoundEntityRef, path: string): Promise<string>;
// (undocumented)
getStorageUrl(): Promise<string>;
// (undocumented)
identityApi: IdentityApi;
syncEntityDocs(
entityId: CompoundEntityRef,
logHandler?: (line: string) => void,
Expand Down
7 changes: 1 addition & 6 deletions plugins/techdocs/dev/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,7 @@ import {
techdocsStorageApiRef,
} from '../src';

import {
configApiRef,
discoveryApiRef,
identityApiRef,
} from '@backstage/core-plugin-api';
import { configApiRef, discoveryApiRef } from '@backstage/core-plugin-api';
import { TechDocsReaderPageProvider } from '@backstage/plugin-techdocs-react';
import { Header, Page, TabbedLayout } from '@backstage/core-components';

Expand Down Expand Up @@ -135,7 +131,6 @@ createDevApp()
deps: {
configApi: configApiRef,
discoveryApi: discoveryApiRef,
identityApi: identityApiRef,
},
factory: () => apiBridge,
})
Expand Down
3 changes: 1 addition & 2 deletions plugins/techdocs/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@
"@material-ui/icons": "^4.9.1",
"@material-ui/lab": "4.0.0-alpha.61",
"@material-ui/styles": "^4.10.0",
"@microsoft/fetch-event-source": "^2.0.1",
"@types/react": "^16.13.1 || ^17.0.0 || ^18.0.0",
"dompurify": "^3.0.0",
"event-source-polyfill": "1.0.25",
"git-url-parse": "^14.0.0",
"jss": "~10.10.0",
"lodash": "^4.17.21",
Expand All @@ -90,7 +90,6 @@
"@testing-library/react": "^15.0.0",
"@testing-library/user-event": "^14.0.0",
"@types/dompurify": "^3.0.0",
"@types/event-source-polyfill": "^1.0.0",
"canvas": "^2.10.2"
},
"peerDependencies": {
Expand Down
5 changes: 1 addition & 4 deletions plugins/techdocs/src/alpha.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import {
createApiFactory,
discoveryApiRef,
fetchApiRef,
identityApiRef,
} from '@backstage/core-plugin-api';
import {
compatWrapper,
Expand All @@ -55,14 +54,12 @@ const techDocsStorageApi = createApiExtension({
deps: {
configApi: configApiRef,
discoveryApi: discoveryApiRef,
identityApi: identityApiRef,
fetchApi: fetchApiRef,
},
factory: ({ configApi, discoveryApi, identityApi, fetchApi }) =>
factory: ({ configApi, discoveryApi, fetchApi }) =>
new TechDocsStorageClient({
configApi,
discoveryApi,
identityApi,
fetchApi,
}),
}),
Expand Down