-
Notifications
You must be signed in to change notification settings - Fork 1
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
[ALT-830] feat: add sdk feature detection object in core #626
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -1,6 +1,7 @@ | |||
import { useEffect, useRef, useState } from 'react'; | |||
import { | |||
doesMismatchMessageSchema, | |||
features, |
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.
i think we should really think carefully about what we name this as this one time decision will have a large impact if we ever want to rename it
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.
I think this is reasonable name. Another slight variation is sdkFeatures
, but no strong feelings on it.
76801b2
to
324bd62
Compare
@@ -0,0 +1 @@ | |||
export const features = {}; |
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.
Should a type be defined now to provide guidance on how developers should add features to this object?
Maybe a type like Record<string, boolean | Record<string, boolean>
could be a starting point?
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.
I made it more generic and unopiniated for now so right now it's typed as Record<string | unknown>
since technically people can just add objects as well.
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.
Overall looks good, left one request change, lmkwyt
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.
I feel like this belongs more in the experiences-sdk-react than in core. There might be a time where we have multiple sdks and they might have diverse features.
@@ -0,0 +1 @@ | |||
export const features = {}; |
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.
A good comment explaining what this file is for would be helpful
@@ -1,6 +1,7 @@ | |||
import { useEffect, useRef, useState } from 'react'; | |||
import { | |||
doesMismatchMessageSchema, | |||
features, |
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.
I think this is reasonable name. Another slight variation is sdkFeatures
, but no strong feelings on it.
324bd62
to
c40ea80
Compare
c40ea80
to
de7a1b1
Compare
Hey @ryunsong-contentful , Can we get this merged? My ticket is dependent on this one. https://contentful.atlassian.net/browse/SPA-2018 Thanks |
Purpose
Create sdk feature detection object in core. Then send it as a post message to the ui