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

Suspend event deliveries until middlewares are ready #552

Merged
merged 2 commits into from
Jul 26, 2022
Merged

Suspend event deliveries until middlewares are ready #552

merged 2 commits into from
Jul 26, 2022

Conversation

zikaari
Copy link
Contributor

@zikaari zikaari commented Jul 20, 2022

This patch fixes an important issue where a middleware will miss out
on augmenting any events while it's being prepped up. Those events will
reach all the destinations as-is without going through the said
middleware.

With this patch, addSourceMiddleware is now classified as a critical
task, and thus will seize all event deliveries until all middlewares are
in an operational state. The suspension happens internally, and the
public API is unaffected.

@changeset-bot
Copy link

changeset-bot bot commented Jul 20, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: b3dd757

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@segment/analytics-next Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@zikaari zikaari force-pushed the suspend_until_middlewares_ready branch from cac3e8b to 8f44a30 Compare July 20, 2022 23:45
@silesky
Copy link
Contributor

silesky commented Jul 21, 2022 via email •
edited
Loading

danieljackins
danieljackins reviewed Jul 21, 2022
View reviewed changes
Copy link
Contributor

@danieljackins danieljackins left a comment

Choose a reason for hiding this comment

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

implementation looks good to me!

silesky
silesky reviewed Jul 21, 2022
View reviewed changes
packages/browser/src/core/task/task-group.ts Outdated
@@ -0,0 +1,22 @@
export class TaskGroup {
private promise!: Promise<void>
Copy link
Contributor

@silesky silesky Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can we have tests that cover this? (We could also write them at the integration level).

silesky
silesky reviewed Jul 21, 2022
View reviewed changes
packages/browser/src/core/task/task-group.ts Outdated
count = 0

done = () => this.promise
run = <Operation extends (...args: any[]) => any>(
Copy link
Contributor

@silesky silesky Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just a non-blocking code review thing.

Another way to type this is “<ReturnT, Operation extends (…args: unknown[]) => ReturnT>(op: Operation): ReturnT

That way, we can save using the ReturnType helper, since we’re already making it ourselves.

Again, just a very mild suggestion!

@zikaari zikaari force-pushed the suspend_until_middlewares_ready branch 3 times, most recently from 5f3d4ce to e8e8fba Compare July 21, 2022 22:06
@zikaari zikaari marked this pull request as ready for review July 21, 2022 22:17
@zikaari zikaari requested review from danieljackins and silesky July 21, 2022 22:17
silesky
silesky reviewed Jul 22, 2022
View reviewed changes
packages/browser/src/core/task/task-group.ts Outdated
run: (op) => {
const returnValue = op()

if (returnValue instanceof Promise) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@chrisradek alerted me to the possible gotchas of “instanceof Promise” (does not work in all environments) — there should be an “isPromise” or “isThenable” helper in utils that we used 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.

Fixed up

@zikaari zikaari force-pushed the suspend_until_middlewares_ready branch from e8e8fba to 93d3a62 Compare July 22, 2022 18:16
@zikaari zikaari force-pushed the suspend_until_middlewares_ready branch from 93d3a62 to dd477cf Compare July 22, 2022 20:06
chrisradek
chrisradek reviewed Jul 25, 2022
View reviewed changes
packages/browser/src/core/task/task-group.ts
taskCompletionPromise = new Promise((res) => (resolvePromise = res))
}

returnValue.finally(() => --count === 0 && resolvePromise())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you confirm that the polyfill we use in the standalone version (CDN version) includes Promise.finally? I think this is the 1st time we'd be using Promise.finally outside npm-specific code.

Copy link
Contributor Author

@zikaari zikaari Jul 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't find any polyfilling at all anywhere. Tried to follow our build pipeline till the end, nothing, and no reference to core-js either.

Copy link
Contributor

Choose a reason for hiding this comment

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

We only polyfill for the CDN version but we load a file here depending on if we determine we should polyfill -

analytics-next/packages/browser/src/browser/standalone.ts

Lines 57 to 63 in aa3c901

if (shouldPolyfill()) {
// load polyfills in order to get AJS to work with old browsers
const script = document.createElement('script')
script.setAttribute(
'src',
'https://cdnjs.cloudflare.com/ajax/libs/babel-polyfill/7.7.0/polyfill.min.js'
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The polyfill works and makes finally work as expected. Tested in IE11 which has no Promise support at all, but once the polyfill script is executed in the console, Promise and finally work nominally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet, thanks!

chrisradek
chrisradek approved these changes Jul 25, 2022
View reviewed changes
Copy link
Contributor

@chrisradek chrisradek left a comment

Choose a reason for hiding this comment

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

Overall looks good to me! Just want to make sure using Promise.finally will be safe! Alternatively could use await with try/finally since typescript will generate that code - but I do think what you have looks cleaner.

@zikaari zikaari merged commit 6741775 into master Jul 26, 2022
@zikaari zikaari deleted the suspend_until_middlewares_ready branch July 26, 2022 16:57
@github-actions github-actions bot mentioned this pull request Jul 26, 2022
Merged
@chrisradek chrisradek mentioned this pull request Sep 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisradek chrisradek chrisradek approved these changes

@silesky silesky silesky left review comments

@danieljackins danieljackins danieljackins approved these changes

@pooyaj pooyaj Awaiting requested review from pooyaj

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zikaari @silesky @danieljackins @chrisradek