ref(app-platform): Add integration features to SentryAppDetailsModal

[MeredithAnya]

Depends on #13377

[mnoble]

We should add some tests ensuring the installation button is disabled, based on the IntegrationFeature.

[mnoble]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions (do you know why?). Then we're adding defaultFeatureGateComponents to it. Then we're calling the first item in that list? Is the push in case it returns an empty list?

[MeredithAnya]

So the defaultFeatureGateComponent list for if you are just running sentry to make this compatible for on-prem people.

Is the push in case it returns an empty list?

This is so we use the pretty markdown and plan specific information for sentry.io

I forget exactly know how the HookStore works so maybe @evanpurkhiser can shed light on the 'returns list of functions' question.

[scefali]

@MeredithAnya I agree with @mnoble that the code makes this a bit confusing to understand what's going on, though I see this is used other places in the code base. I'm wondering if there is a clearer way to get the same behavior. Here is my suggestion:

const featureHook = HookStore.get('integrations:feature-gates')[0] || defaultFeatureGateComponents;
const {FeatureList, IntegrationFeatures} = featureHook();

This also avoids mutating the array that is stored in the HookStore with key integrations:feature-gates. The mutation probably does not have any consequences in this case but in general, it's better to avoid mutation unless that is the explicit goal.

[evanpurkhiser]

This was my doing.

Yes we push it onto the end in the case that it's empty.

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||defaultFeatureGateComponents;

This would need to be written as

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||(()=>defaultFeatureGateComponents);

Since we call the hookstore function to get our components back.

[evanpurkhiser]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions

This is historically just the convention that we have typically used for hookstore. I think it's not great considering there are actually a lot of times where we do not want a list. In fact, if you look around the code base, I would say a majority of the times we just pop off the first function and call it.

[scefali]

@evanpurkhiser Does it make sense to make a getFirst method on the HookStore? Probably outside the scope of this PR but if what you are saying is true then getting the full array is just a hassle.

EDIT:
Maybe even have a getFirstAndCall method so we can take care of calling the method as well.

[evanpurkhiser]

It would probably make more sense to re-evaluate how we're using hookstore. Not sure how much you've seen it yet, but it's basically a bridge between sentry and getsentry, a way to add frontend features specific to the SASS product

[mnoble]

I'd update that one section to what @scefali suggested, get this on staging again, and then I think we're good.

[MeredithAnya]

Depends on #13377

[mnoble]

We should add some tests ensuring the installation button is disabled, based on the IntegrationFeature.

[mnoble]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions (do you know why?). Then we're adding defaultFeatureGateComponents to it. Then we're calling the first item in that list? Is the push in case it returns an empty list?

[MeredithAnya]

So the defaultFeatureGateComponent list for if you are just running sentry to make this compatible for on-prem people.

Is the push in case it returns an empty list?

This is so we use the pretty markdown and plan specific information for sentry.io

I forget exactly know how the HookStore works so maybe @evanpurkhiser can shed light on the 'returns list of functions' question.

[scefali]

@MeredithAnya I agree with @mnoble that the code makes this a bit confusing to understand what's going on, though I see this is used other places in the code base. I'm wondering if there is a clearer way to get the same behavior. Here is my suggestion:

const featureHook = HookStore.get('integrations:feature-gates')[0] || defaultFeatureGateComponents;
const {FeatureList, IntegrationFeatures} = featureHook();

This also avoids mutating the array that is stored in the HookStore with key integrations:feature-gates. The mutation probably does not have any consequences in this case but in general, it's better to avoid mutation unless that is the explicit goal.

[evanpurkhiser]

This was my doing.

Yes we push it onto the end in the case that it's empty.

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||defaultFeatureGateComponents;

This would need to be written as

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||(()=>defaultFeatureGateComponents);

Since we call the hookstore function to get our components back.

[evanpurkhiser]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions

This is historically just the convention that we have typically used for hookstore. I think it's not great considering there are actually a lot of times where we do not want a list. In fact, if you look around the code base, I would say a majority of the times we just pop off the first function and call it.

[scefali]

@evanpurkhiser Does it make sense to make a getFirst method on the HookStore? Probably outside the scope of this PR but if what you are saying is true then getting the full array is just a hassle.

EDIT:
Maybe even have a getFirstAndCall method so we can take care of calling the method as well.

[evanpurkhiser]

It would probably make more sense to re-evaluate how we're using hookstore. Not sure how much you've seen it yet, but it's basically a bridge between sentry and getsentry, a way to add frontend features specific to the SASS product

[mnoble]

I'd update that one section to what @scefali suggested, get this on staging again, and then I think we're good.

[MeredithAnya]

Depends on #13377

[mnoble]

We should add some tests ensuring the installation button is disabled, based on the IntegrationFeature.

[mnoble]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions (do you know why?). Then we're adding defaultFeatureGateComponents to it. Then we're calling the first item in that list? Is the push in case it returns an empty list?

[MeredithAnya]

So the defaultFeatureGateComponent list for if you are just running sentry to make this compatible for on-prem people.

Is the push in case it returns an empty list?

This is so we use the pretty markdown and plan specific information for sentry.io

I forget exactly know how the HookStore works so maybe @evanpurkhiser can shed light on the 'returns list of functions' question.

[scefali]

@MeredithAnya I agree with @mnoble that the code makes this a bit confusing to understand what's going on, though I see this is used other places in the code base. I'm wondering if there is a clearer way to get the same behavior. Here is my suggestion:

const featureHook = HookStore.get('integrations:feature-gates')[0] || defaultFeatureGateComponents;
const {FeatureList, IntegrationFeatures} = featureHook();

This also avoids mutating the array that is stored in the HookStore with key integrations:feature-gates. The mutation probably does not have any consequences in this case but in general, it's better to avoid mutation unless that is the explicit goal.

[evanpurkhiser]

This was my doing.

Yes we push it onto the end in the case that it's empty.

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||defaultFeatureGateComponents;

This would need to be written as

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||(()=>defaultFeatureGateComponents);

Since we call the hookstore function to get our components back.

[evanpurkhiser]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions

This is historically just the convention that we have typically used for hookstore. I think it's not great considering there are actually a lot of times where we do not want a list. In fact, if you look around the code base, I would say a majority of the times we just pop off the first function and call it.

[scefali]

@evanpurkhiser Does it make sense to make a getFirst method on the HookStore? Probably outside the scope of this PR but if what you are saying is true then getting the full array is just a hassle.

EDIT:
Maybe even have a getFirstAndCall method so we can take care of calling the method as well.

[evanpurkhiser]

It would probably make more sense to re-evaluate how we're using hookstore. Not sure how much you've seen it yet, but it's basically a bridge between sentry and getsentry, a way to add frontend features specific to the SASS product

[mnoble]

I'd update that one section to what @scefali suggested, get this on staging again, and then I think we're good.

[MeredithAnya]

Depends on #13377

[mnoble]

We should add some tests ensuring the installation button is disabled, based on the IntegrationFeature.

[mnoble]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions (do you know why?). Then we're adding defaultFeatureGateComponents to it. Then we're calling the first item in that list? Is the push in case it returns an empty list?

[MeredithAnya]

So the defaultFeatureGateComponent list for if you are just running sentry to make this compatible for on-prem people.

Is the push in case it returns an empty list?

This is so we use the pretty markdown and plan specific information for sentry.io

I forget exactly know how the HookStore works so maybe @evanpurkhiser can shed light on the 'returns list of functions' question.

[scefali]

@MeredithAnya I agree with @mnoble that the code makes this a bit confusing to understand what's going on, though I see this is used other places in the code base. I'm wondering if there is a clearer way to get the same behavior. Here is my suggestion:

const featureHook = HookStore.get('integrations:feature-gates')[0] || defaultFeatureGateComponents;
const {FeatureList, IntegrationFeatures} = featureHook();

This also avoids mutating the array that is stored in the HookStore with key integrations:feature-gates. The mutation probably does not have any consequences in this case but in general, it's better to avoid mutation unless that is the explicit goal.

[evanpurkhiser]

This was my doing.

Yes we push it onto the end in the case that it's empty.

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||defaultFeatureGateComponents;

This would need to be written as

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||(()=>defaultFeatureGateComponents);

Since we call the hookstore function to get our components back.

[evanpurkhiser]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions

This is historically just the convention that we have typically used for hookstore. I think it's not great considering there are actually a lot of times where we do not want a list. In fact, if you look around the code base, I would say a majority of the times we just pop off the first function and call it.

[scefali]

@evanpurkhiser Does it make sense to make a getFirst method on the HookStore? Probably outside the scope of this PR but if what you are saying is true then getting the full array is just a hassle.

EDIT:
Maybe even have a getFirstAndCall method so we can take care of calling the method as well.

[evanpurkhiser]

It would probably make more sense to re-evaluate how we're using hookstore. Not sure how much you've seen it yet, but it's basically a bridge between sentry and getsentry, a way to add frontend features specific to the SASS product

[mnoble]

I'd update that one section to what @scefali suggested, get this on staging again, and then I think we're good.

[MeredithAnya]

Depends on #13377

[mnoble]

We should add some tests ensuring the installation button is disabled, based on the IntegrationFeature.

[mnoble]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions (do you know why?). Then we're adding defaultFeatureGateComponents to it. Then we're calling the first item in that list? Is the push in case it returns an empty list?

[MeredithAnya]

So the defaultFeatureGateComponent list for if you are just running sentry to make this compatible for on-prem people.

Is the push in case it returns an empty list?

This is so we use the pretty markdown and plan specific information for sentry.io

I forget exactly know how the HookStore works so maybe @evanpurkhiser can shed light on the 'returns list of functions' question.

[scefali]

@MeredithAnya I agree with @mnoble that the code makes this a bit confusing to understand what's going on, though I see this is used other places in the code base. I'm wondering if there is a clearer way to get the same behavior. Here is my suggestion:

const featureHook = HookStore.get('integrations:feature-gates')[0] || defaultFeatureGateComponents;
const {FeatureList, IntegrationFeatures} = featureHook();

This also avoids mutating the array that is stored in the HookStore with key integrations:feature-gates. The mutation probably does not have any consequences in this case but in general, it's better to avoid mutation unless that is the explicit goal.

[evanpurkhiser]

This was my doing.

Yes we push it onto the end in the case that it's empty.

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||defaultFeatureGateComponents;

This would need to be written as

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||(()=>defaultFeatureGateComponents);

Since we call the hookstore function to get our components back.

[evanpurkhiser]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions

This is historically just the convention that we have typically used for hookstore. I think it's not great considering there are actually a lot of times where we do not want a list. In fact, if you look around the code base, I would say a majority of the times we just pop off the first function and call it.

[scefali]

@evanpurkhiser Does it make sense to make a getFirst method on the HookStore? Probably outside the scope of this PR but if what you are saying is true then getting the full array is just a hassle.

EDIT:
Maybe even have a getFirstAndCall method so we can take care of calling the method as well.

[evanpurkhiser]

It would probably make more sense to re-evaluate how we're using hookstore. Not sure how much you've seen it yet, but it's basically a bridge between sentry and getsentry, a way to add frontend features specific to the SASS product

[mnoble]

I'd update that one section to what @scefali suggested, get this on staging again, and then I think we're good.

[MeredithAnya]

Depends on #13377

[mnoble]

We should add some tests ensuring the installation button is disabled, based on the IntegrationFeature.

[mnoble]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions (do you know why?). Then we're adding defaultFeatureGateComponents to it. Then we're calling the first item in that list? Is the push in case it returns an empty list?

[MeredithAnya]

So the defaultFeatureGateComponent list for if you are just running sentry to make this compatible for on-prem people.

Is the push in case it returns an empty list?

This is so we use the pretty markdown and plan specific information for sentry.io

I forget exactly know how the HookStore works so maybe @evanpurkhiser can shed light on the 'returns list of functions' question.

[scefali]

@MeredithAnya I agree with @mnoble that the code makes this a bit confusing to understand what's going on, though I see this is used other places in the code base. I'm wondering if there is a clearer way to get the same behavior. Here is my suggestion:

const featureHook = HookStore.get('integrations:feature-gates')[0] || defaultFeatureGateComponents;
const {FeatureList, IntegrationFeatures} = featureHook();

This also avoids mutating the array that is stored in the HookStore with key integrations:feature-gates. The mutation probably does not have any consequences in this case but in general, it's better to avoid mutation unless that is the explicit goal.

[evanpurkhiser]

This was my doing.

Yes we push it onto the end in the case that it's empty.

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||defaultFeatureGateComponents;

This would need to be written as

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||(()=>defaultFeatureGateComponents);

Since we call the hookstore function to get our components back.

[evanpurkhiser]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions

This is historically just the convention that we have typically used for hookstore. I think it's not great considering there are actually a lot of times where we do not want a list. In fact, if you look around the code base, I would say a majority of the times we just pop off the first function and call it.

[scefali]

@evanpurkhiser Does it make sense to make a getFirst method on the HookStore? Probably outside the scope of this PR but if what you are saying is true then getting the full array is just a hassle.

EDIT:
Maybe even have a getFirstAndCall method so we can take care of calling the method as well.

[evanpurkhiser]

It would probably make more sense to re-evaluate how we're using hookstore. Not sure how much you've seen it yet, but it's basically a bridge between sentry and getsentry, a way to add frontend features specific to the SASS product

[mnoble]

I'd update that one section to what @scefali suggested, get this on staging again, and then I think we're good.

[MeredithAnya]

Depends on #13377

[mnoble]

We should add some tests ensuring the installation button is disabled, based on the IntegrationFeature.

[mnoble]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions (do you know why?). Then we're adding defaultFeatureGateComponents to it. Then we're calling the first item in that list? Is the push in case it returns an empty list?

[MeredithAnya]

So the defaultFeatureGateComponent list for if you are just running sentry to make this compatible for on-prem people.

Is the push in case it returns an empty list?

This is so we use the pretty markdown and plan specific information for sentry.io

I forget exactly know how the HookStore works so maybe @evanpurkhiser can shed light on the 'returns list of functions' question.

[scefali]

@MeredithAnya I agree with @mnoble that the code makes this a bit confusing to understand what's going on, though I see this is used other places in the code base. I'm wondering if there is a clearer way to get the same behavior. Here is my suggestion:

const featureHook = HookStore.get('integrations:feature-gates')[0] || defaultFeatureGateComponents;
const {FeatureList, IntegrationFeatures} = featureHook();

This also avoids mutating the array that is stored in the HookStore with key integrations:feature-gates. The mutation probably does not have any consequences in this case but in general, it's better to avoid mutation unless that is the explicit goal.

[evanpurkhiser]

This was my doing.

Yes we push it onto the end in the case that it's empty.

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||defaultFeatureGateComponents;

This would need to be written as

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||(()=>defaultFeatureGateComponents);

Since we call the hookstore function to get our components back.

[evanpurkhiser]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions

This is historically just the convention that we have typically used for hookstore. I think it's not great considering there are actually a lot of times where we do not want a list. In fact, if you look around the code base, I would say a majority of the times we just pop off the first function and call it.

[scefali]

@evanpurkhiser Does it make sense to make a getFirst method on the HookStore? Probably outside the scope of this PR but if what you are saying is true then getting the full array is just a hassle.

EDIT:
Maybe even have a getFirstAndCall method so we can take care of calling the method as well.

[evanpurkhiser]

It would probably make more sense to re-evaluate how we're using hookstore. Not sure how much you've seen it yet, but it's basically a bridge between sentry and getsentry, a way to add frontend features specific to the SASS product

[mnoble]

I'd update that one section to what @scefali suggested, get this on staging again, and then I think we're good.

[MeredithAnya]

Depends on #13377

[mnoble]

We should add some tests ensuring the installation button is disabled, based on the IntegrationFeature.

[mnoble]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions (do you know why?). Then we're adding defaultFeatureGateComponents to it. Then we're calling the first item in that list? Is the push in case it returns an empty list?

[MeredithAnya]

So the defaultFeatureGateComponent list for if you are just running sentry to make this compatible for on-prem people.

Is the push in case it returns an empty list?

This is so we use the pretty markdown and plan specific information for sentry.io

I forget exactly know how the HookStore works so maybe @evanpurkhiser can shed light on the 'returns list of functions' question.

[scefali]

@MeredithAnya I agree with @mnoble that the code makes this a bit confusing to understand what's going on, though I see this is used other places in the code base. I'm wondering if there is a clearer way to get the same behavior. Here is my suggestion:

const featureHook = HookStore.get('integrations:feature-gates')[0] || defaultFeatureGateComponents;
const {FeatureList, IntegrationFeatures} = featureHook();

This also avoids mutating the array that is stored in the HookStore with key integrations:feature-gates. The mutation probably does not have any consequences in this case but in general, it's better to avoid mutation unless that is the explicit goal.

[evanpurkhiser]

This was my doing.

Yes we push it onto the end in the case that it's empty.

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||defaultFeatureGateComponents;

This would need to be written as

constfeatureHook=HookStore.get('integrations:feature-gates')[0]||(()=>defaultFeatureGateComponents);

Since we call the hookstore function to get our components back.

[evanpurkhiser]

I'm not sure what the above code is doing. It looks like HookStore returns a list of functions

This is historically just the convention that we have typically used for hookstore. I think it's not great considering there are actually a lot of times where we do not want a list. In fact, if you look around the code base, I would say a majority of the times we just pop off the first function and call it.

[scefali]

@evanpurkhiser Does it make sense to make a getFirst method on the HookStore? Probably outside the scope of this PR but if what you are saying is true then getting the full array is just a hassle.

EDIT:
Maybe even have a getFirstAndCall method so we can take care of calling the method as well.

[evanpurkhiser]

It would probably make more sense to re-evaluate how we're using hookstore. Not sure how much you've seen it yet, but it's basically a bridge between sentry and getsentry, a way to add frontend features specific to the SASS product

[mnoble]

I'd update that one section to what @scefali suggested, get this on staging again, and then I think we're good.