Codenotify: Notifying subscribers in CODENOTIFY files for diff 185c154...d6a939b.

No notifications.

The global jscontext is already super bloated, does it have to be in there or could we request it dynamically if the user visits a page that requires it?

The global jscontext is already super bloated, does it have to be in there or could we request it dynamically if the user visits a page that requires it?

I agree and I initially thought of requesting it via the GraphQL endpoint but I wasn't sure the tradeoff made sense.

We'll basically be making the Site query to get one field that'll be used in the User settings page and in the Batch Change details page.

Super delayed feedback here, but just wanted to share, I think I'd also prefer this sort of thing to come from the GraphQL API rather than the JSContext object. I don't believe we've explicitly stated guidance on this sort of thing anywhere, and this definitely isn't true for everything that's in there today, but generally I consider the things we put in JSContext to be:

• critical to the frontend web client rendering properly on initial load
• almost always able to be treated as a constant over the lifetime of the user visit

The fields like batchChangesEnabled, experimentalFeatures, and licenseInfo really exemplify this to me. batchChangesRolloutWindows may be safely treated as a constant, but I wouldn't consider it essential information to the frontend; it's possible a user who visits Sourcegraph never once visits a page where we even need to know that information. The main benefit to putting stuff here seems to be avoiding the network requests waterfall. Historically it probably also had the benefit of preventing redundant queries for something that was unchanging, but today we have the Apollo cache to mitigate that instead. This field on the site config could be safely requested in parallel, and I wouldn't expect it to require any dependent follow-up queries, so the anti-waterfalls benefit isn't really relevant. I'd also argue batchChangesDisableWebhooksWarning and batchChangesWebhookLogsEnabled don't belong here anymore, but I suspect they were added at a time when we didn't have good caching practices established. The GraphQL API provides better organization and predictability about this field, compared to the flat grab bag of keys in the JSContext, and it would allow us to defer requesting it until we're sure we even need it.

It's not a big deal to leave it here, just wanted to share my two cents since I realize this isn't a topic that's well documented (and I'll make a note to raise this as a topic in need of clearer guidance/explanation at frontend crew!!), but maybe it's worth filing an issue to refactor to a GraphQL query for a rainy day. Bonus points if we can update batchChangesDisableWebhooksWarning and batchChangesWebhookLogsEnabled, too. :)

Agree with Kelli here on the reasoning :)

Super delayed feedback here, but just wanted to share, I think I'd also prefer this sort of thing to come from the GraphQL API rather than the JSContext object. I don't believe we've explicitly stated guidance on this sort of thing anywhere, and this definitely isn't true for everything that's in there today, but generally I consider the things we put in JSContext to be:

• critical to the frontend web client rendering properly on initial load
• almost always able to be treated as a constant over the lifetime of the user visit

The fields like batchChangesEnabled, experimentalFeatures, and licenseInfo really exemplify this to me. batchChangesRolloutWindows may be safely treated as a constant, but I wouldn't consider it essential information to the frontend; it's possible a user who visits Sourcegraph never once visits a page where we even need to know that information. The main benefit to putting stuff here seems to be avoiding the network requests waterfall. Historically it probably also had the benefit of preventing redundant queries for something that was unchanging, but today we have the Apollo cache to mitigate that instead. This field on the site config could be safely requested in parallel, and I wouldn't expect it to require any dependent follow-up queries, so the anti-waterfalls benefit isn't really relevant. I'd also argue batchChangesDisableWebhooksWarning and batchChangesWebhookLogsEnabled don't belong here anymore, but I suspect they were added at a time when we didn't have good caching practices established. The GraphQL API provides better organization and predictability about this field, compared to the flat grab bag of keys in the JSContext, and it would allow us to defer requesting it until we're sure we even need it.

It's not a big deal to leave it here, just wanted to share my two cents since I realize this isn't a topic that's well documented (and I'll make a note to raise this as a topic in need of clearer guidance/explanation at frontend crew!!), but maybe it's worth filing an issue to refactor to a GraphQL query for a rainy day. Bonus points if we can update batchChangesDisableWebhooksWarning and batchChangesWebhookLogsEnabled, too. :)

As discussed, #51752 takes care of this and removes the rollout window configuration from JSContext.
Thank you all.