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

Fix plugin pipeline race condition #1121

Merged
merged 4 commits into from
Aug 19, 2024
Merged

Fix plugin pipeline race condition #1121

merged 4 commits into from
Aug 19, 2024

Conversation

silesky
Copy link
Contributor

@silesky silesky commented Jul 23, 2024

Fixes a race condition in our plugin pipeline:

#1120

There is currently inconsistencies between whether or not plugins are loaded in the Plugins[] array, or whether or not they are registered using .register. If they are loaded using .register, we don't wait wait for .load to finish resolving before invoking methods in that plugin. This is specifically an issue around plugins that are manually registered before analytics is initialized (i.e., most uses of analytics.register). The bug doesn't affect plugins loaded via action destinations, only plugins that are manually registered using analytics.register.

Bug repro:
Currently, this results in "page()!" being logged before ".load()!".

 export const BugPlugin: Plugin = {
   type: "enrichment",
   name: "My Buggy Plugin",
   version: "1.0.0",
   load: async () => {
     await new Promise((resolve) => setTimeout(() => resolve("data"), 3000));
     console.log(
       ".load()! I should come before page"
     );
   },
   page: (ctx) => {
     console.log("page()! I should come after .load()")
     return ctx;
   },
   isLoaded: () => true,
 } 
  analytics.register(BugPlugin);
  analytics.page()
  analytics.load({ writeKey: 'MY_WRITEKEY' })
}

Copy link

changeset-bot bot commented Jul 23, 2024

🦋 Changeset detected

Latest commit: 0ae8a07

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

bsneed
bsneed previously approved these changes Jul 23, 2024
@silesky silesky force-pushed the fix-plugin-pipeline branch from 8d5c27e to 0ae8a07 Compare August 16, 2024 20:21
}
} catch (err) {
console.error('Failed to create PluginFactory', remotePlugin)
throw err
Copy link
Contributor Author

@silesky silesky Aug 16, 2024

Choose a reason for hiding this comment

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

The only thing I did here was wrap the entire thing in a try/catch (for debugging)

@silesky silesky requested a review from danieljackins August 16, 2024 20:58
@silesky silesky merged commit d98dcd2 into master Aug 19, 2024
5 checks passed
@silesky silesky deleted the fix-plugin-pipeline branch August 19, 2024 16:51
@github-actions github-actions bot mentioned this pull request Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants