Skip to content
This repository has been archived by the owner on Nov 4, 2021. It is now read-only.

Slightly change the way changing team_id by plugins is illegal #396

Merged
merged 3 commits into from
May 19, 2021

Conversation

Twixes
Copy link
Collaborator

@Twixes Twixes commented May 19, 2021

Changes

So this PR adds the feature to batch processing like #394, but instead of silently restoring the correct team_id (unlike master which nullifies the event), it restores it AND throws an IllegalOperationError. This way the impact of illegally operating plugins is minimized, but the problem is still communicated as an error. Closes #394.

Checklist

  • Jest tests

Copy link
Contributor

@neilkakkar neilkakkar left a comment

Choose a reason for hiding this comment

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

This is great!

src/worker/plugins/run.ts Outdated Show resolved Hide resolved
@Twixes Twixes added the bump patch Bump patch version when this PR gets merged label May 19, 2021
export class IllegalOperationError extends Error {
name = 'IllegalOperationError'

constructor(operation: string) {
Copy link
Contributor

@yakkomajuri yakkomajuri May 19, 2021

Choose a reason for hiding this comment

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

not even sure it's a nit, but I don't think you need the constructor if it just calls super. The constructor of the parent class should be called anyway if this is omitted. Fine if you like the explicitness though.

@mariusandra
Copy link
Collaborator

Great work everyone in getting to this solution! LGTM.

@Twixes Twixes merged commit 2e83a0e into master May 19, 2021
@Twixes Twixes deleted the slight-team-id-ensuring-change branch May 19, 2021 19:44
fuziontech pushed a commit to PostHog/posthog that referenced this pull request Oct 12, 2021
… is illegal (PostHog/plugin-server#396)

* Slightly change the way changing team_id by plugins is illegal

* Prettier

* Remove debug console.log
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants