ref(nextjs): Simplify adding default integrations when initializing SDK #5702
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report 📦
|
lobsterkatie
force-pushed
the
kmclb-nextjs-simplify-adding-default-integrations
branch
from
0bd5df0 to
5b65fff
Compare
AbhiPrasad
reviewed
Sep 7, 2022
| return [defaultBrowserTracingIntegration]; | ||
| } | ||
| return addOrUpdateIntegration(defaultBrowserTracingIntegration, userIntegrations, { | ||
| 'options.routingInstrumentation': nextRouterInstrumentation, |
There was a problem hiding this comment.
With this API, couldn't we collide if integrations have the same option name?
There was a problem hiding this comment.
It's the same API as before, just a slightly simpler implementation - it still filters on integration name. See the const userInstance = userIntegrations.find(integration => integration.name === defaultIntegrationInstance.name); line in addOrUpdateIntegrationInArray.
What it doesn't protect against is duplicates (the way the main integration logic does), but hopefully users won't be giving us two copies of the same integration.
lobsterkatie
force-pushed
the
kmclb-nextjs-simplify-adding-default-integrations
branch
2 times, most recently
from
3fb85ca to
4a9c167
Compare
lobsterkatie
force-pushed
the
kmclb-nextjs-simplify-adding-default-integrations
branch
from
4a9c167 to
0414730
Compare
AbhiPrasad
approved these changes
Sep 9, 2022
lobsterkatie
deleted the
kmclb-nextjs-simplify-adding-default-integrations
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on adding a
RequestDataintegration to the nextjs SDK, I found that the functions we use to set default integrations in the SDK were a little hard to parse, primarily because of a docstring that didn't make any sense and vague and/or slightly inaccurate function and variable names. We also have been making things more complicated than necessary, in thata) The existing functions in
utils/userIntegrations.tsuse the given default integration instance if no user instance is found. Meanwhile, the two index files do the same in the case that no user integrations are provided at all. But those two conditions aren't logically independent, so by simply providing a default (empty) value for the second case, we can rely on the first check to ensure the default instance is used. (See the now-removedif (options.integrations)check in both of the index files.)b) The options we want to force onto any user instance we find have been living in a nested object which really doesn’t need to be nested. (See the
Options- nowForcedIntegrationOptions- type inutils/userIntegrations.ts.)c) We've essentially been manually implementing
Array.find, which there's no reason for us to do. (See thefor-loop inaddIntegrationToArray- nowaddOrUpdateIntegrationInArray- inutils/userIntegrations.ts.)This fixes those problems, rewrites the docstring, and renames a bunch of variables. It also
integrationsarray when testing an individual integration type.setNestedKey.For ease of reviewing, the docstring/renaming changes have been separated into a separate commit from the logic changes, test reorg, and bugfix.