-
-
Notifications
You must be signed in to change notification settings - Fork 338
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: assign default options before enableNative check #1007
Conversation
Note sure how CI breaks: This change is unrelated |
This actually breaks because the function returns a |
src/js/sdk.ts
Outdated
@@ -71,10 +71,10 @@ export function init( | |||
}, | |||
}) | |||
); | |||
options = assignDefaultOptions(options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options = assignDefaultOptions(options); | |
options = { | |
...DEFAULT_OPTIONS, | |
...options | |
} |
Instead of using a function call, I would do this and define the DEFAULT_OPTIONS
as a constant object, this way typescript knows for sure that defaultIntegrations
is defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file needs a massive refactor/clean now that I look at it, but I'll take care it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea 👍
addressed in the most recent commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh the build is failing still, this would be fixed if you move the default options setting code before the if statement that sets the default integrations above it. Otherwise everything else looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotchya, fixed that & the build is passing now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
📢 Type of change
📜 Description
the changes i made in #993 result in
deviceContext
not initializing by default, since the defaultenableNative:true
isn't set until right after💡 Motivation and Context
#993 (comment)
💚 How did you test it?
existing tests
📝 Checklist
🔮 Next steps