-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
fix: move from event-source-polyfill to fetch-event-source #24861
Conversation
Changed Packages
|
Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
2799245
to
cbebad1
Compare
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.
Nice!
Co-authored-by: Aramis Sennyey <159921952+aramissennyeydd@users.noreply.github.com> Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
}); | ||
eventSource.addEventListener('error', event => { | ||
subscriber.error(event); | ||
fetchEventSource(url, { |
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.
Does this automatically cleanup? We seem to missing the eventSource.close
from before.
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.
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
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.
It uses a signal
instead, which is the more modern API. We should implement it using that instead.
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.
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.
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..
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.
Hey 👋 thanks for this, just a few small things! 🙏
@@ -465,7 +464,6 @@ export class TechDocsStorageClient implements TechDocsStorageApi_2 { | |||
constructor(options: { | |||
configApi: Config; | |||
discoveryApi: DiscoveryApi; | |||
identityApi: IdentityApi; |
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.
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.
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.
I'm not 100% sure the way I marked it as deprecated is fine (couldn't find a similar example). Can you check?
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.
It needs to use a doc comment, i.e. /** @deprecated ... */
. Let's keep the IdentityApi
type as well, instead of any
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.
I did the comment, but somehow I can't make it show up in the generated api-report.md.
any hints are welcome
Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
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.
Sweet, thank you! 👍
Just a couple of small fixes
}); | ||
eventSource.addEventListener('error', event => { | ||
subscriber.error(event); | ||
fetchEventSource(url, { |
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.
It uses a signal
instead, which is the more modern API. We should implement it using that instead.
@@ -465,7 +464,6 @@ export class TechDocsStorageClient implements TechDocsStorageApi_2 { | |||
constructor(options: { | |||
configApi: Config; | |||
discoveryApi: DiscoveryApi; | |||
identityApi: IdentityApi; |
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.
It needs to use a doc comment, i.e. /** @deprecated ... */
. Let's keep the IdentityApi
type as well, instead of any
Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
Uffizzi Ephemeral Environment - Virtual ClusterYour cluster
Access the |
Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
seems like e2e is broken now. I tried merging master onto my branch, no success to fix them. I doubt the failure relates to the code, will try to reproduce it locally |
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.
this is amazing, thanks for cleaning it up 🙏
Co-authored-by: Vincenzo Scamporlino <vincenzos@spotify.com> Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
@Rugvip / @benjdlambert can you have another look? |
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.
Nice, thank you! 👍 🎉
Thank you for contributing to Backstage! The changes in this pull request will be part of the |
Hey, I just made a Pull Request!
Replace event-source-polyfill with @microsoft/fetch-event-source so we can pass our fetchApi implementation.
See @Rugvip comment: #23015 (comment)
✔️ Checklist
Signed-off-by
line in the message. (more info)