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

Move context augmentation to Page enrichment #939

Merged
merged 5 commits into from
Aug 31, 2023
Merged

Conversation

zikaari
Copy link
Contributor

@zikaari zikaari commented Aug 25, 2023

Part of https://segment.atlassian.net/browse/LIBWEB-1426

Since Page Enrichment is no longer just enriching "page" events, with page information, I wanted to rename this to something generic like "Context Enrichment", but wasn't certain if this is safe. Open to discussion.

The original implementation suggests something about falling back to "options" if "context" is not there. Not sure what that means, so please help me identify what's the intended outcome of that design so I can double-check that behaviour is not impacted.

@changeset-bot
Copy link

changeset-bot bot commented Aug 25, 2023

🦋 Changeset detected

Latest commit: 4614b24

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

This PR includes changesets to release 3 packages
Name Type
@segment/analytics-next Minor
@segment/analytics-core Patch
@segment/analytics-node 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

packages/browser/src/plugins/page-enrichment/index.ts Outdated Show resolved Hide resolved
packages/browser/src/plugins/page-enrichment/index.ts Outdated Show resolved Hide resolved
packages/core/src/events/interfaces.ts Show resolved Hide resolved
packages/browser/src/plugins/page-enrichment/index.ts Outdated Show resolved Hide resolved
@silesky
Copy link
Contributor

silesky commented Aug 28, 2023

@zikaari This PR is a big improvement architecturally! Thanks for taking this on.

I wanted to rename this to something generic like "Context Enrichment", but wasn't certain if this is safe. Open to discussion.

Cool! Yeah, I see this plugin is about augmenting the event with information from the user's environment-- so maybe EnvironmentAugmentation plugin would be a good candidate -- but "page" can also be interpreted as a kind of word for environment :-)

I mainly don't like the word 'enrichment' because this plugin isn't a type:enrichment plugin -- so it's v confusing.

@zikaari
Copy link
Contributor Author

zikaari commented Aug 28, 2023

@pooyaj Do you know any dangers of renaming a plugin on top of your head? Not just the symbol name, but also Plugin.name property.

If you don't feel confident, we can leave renaming it to some other time.

@zikaari zikaari requested a review from silesky August 28, 2023 23:44
@silesky
Copy link
Contributor

silesky commented Aug 29, 2023

@zikaari change to core package (campaign interface) should have a another changeset 😉

@zikaari zikaari merged commit ee855ba into master Aug 31, 2023
3 checks passed
@zikaari zikaari deleted the move_ctx_augmentation branch August 31, 2023 21:10
@github-actions github-actions bot mentioned this pull request Aug 31, 2023
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.

2 participants