I saw your comment in slack on this, but I still think that we shouldn't add the dependency here, and in /package.json instead.

There are a lot of dependencies in there already that are either web, shared, or something else only, but imported to all of them.

That prevents that we have to make sure not to have two objects of the same name, but no actual equality because they're from different versions.

I am 100% in agreement that we want to move this new dependency into the narrowest scope possible. So if /package.json is the right place to put it, awesome!

But for my own understanding, what's the difference between client/web/package.json and /package.json? Both have the description "The Sourcegraph web app", and I would have assumed that the "more general one" would be in the root of the repository. And that the package.json file just for the Sourcegraph web app would be in client/web... but I take it that isn't the case?

Also, should I ignore this warning? Sounds scary. But then again, I don't quite know what we use the root package.json file for.

% pnpm install @stripe/sourcegraph-js
 ERR_PNPM_ADDING_TO_ROOT  Running this command will add the dependency to the workspace root, which might not be what you want
- if you really meant it, make it explicit by running this command again with the -w flag (or --workspace-root). If you don't
- want to see this warning anymore, you may set the ignore-workspace-root-check setting to true.

I'd be curious about the reason for not currently having any non-workspace dependencies in client/web/package.json. I don't know about any particular best practice that would oppose it, but I'm sure we have some reason, and our current use case doesn't seem that special compared to what we normally do. Were people just lazy and put everything to /package.json? I kinda doubt it.

Having multiple versions of the same dependency is valid in JS land, but can cause issues like instanceof checks suddenly failing. It can also cause the bundle to become much bigger because you suddenly import multiple versions.

I do agree with yall that we should probably use packages by workspace and isolate those more, but since we don't do that today, for consistency I think we should add to the root package. I am not sure why pnpm complains about that though, this is how we always did it :/

I do agree with yall that we should probably use packages by workspace and isolate those more, but since we don't do that today, for consistency I think we should add to the root package ... this is how we always did it :/

Sounds good. I'll defer to your expertise here.

I'm surprised we don't have a helper that wraps fetch nicely and handles auth etc, but I couldn't find one in our codebase, so 🤷

Well, don't worry. We will have a helper that wraps fetch nicely and handles auth... since we'll be adding it a follow-up PR as we add more instances where we need to call the SSC backend through this .api/ssc/proxy endpoint.

Also nit: "resp" is pretty Go-ish. "response" is idiomatic. (If you don't believe me, search our front-end codebase, "response" wins by a landslide)

We have a container component that has a standardized look, could we use that, even if it's slightly different than this one?

This was 100% cargo culted from the CodyManagementPage.module.scss. There is no reason to use a slightly different one. So I'll actually just remove it entirely, since removing the .module.scss file entirely doesn't seem to make any changes, and we only were using the .container property.

https://github.com/sourcegraph/sourcegraph/blob/main/client/web/src/cody/management/CodyManagementPage.module.scss#L15

I would inline these types in cmd/frontend/internal/app/jscontext/jscontext.go, it looks like this code has nothing to do with the schema generator that creates most contents in this package

That sounds reasonable, so I tried to make that change. But by inlining the type, it means we cannot create an instance of it when we need to. (Since the FrontendCodyConfig should only be non-nil IFF dotcom and dotcom.codyProConfig config values are set.

Oh I didn't mean literal inlining (sorry - this term is overloaded), I meant just moving the code in this file at the end of jscontext.go :)

Does that work? I would think condition is only evaluated once and while the feature flag state hasn't been loaded yet from the server, it would default to false (and the second entry in the return array would indicate it's still loading).

Also, I don't think you can (or even should be allowed to) use React hooks outside of components. Even if React/TS allows you, this looks like an anti-pattern. I'd just rely on the config and leave it at that. Then you can check for the feature flag in NewCodyProSubscriptionPage.

Does that work?

It certainly works locally 🤷 . But if we "should" do this, is another question entirely.

I'm happy to remove it. But in case you had forgotten David, I literally added this feature flag because you and Naman insisted upon it. And initially started with using site-config to gate these pages 😠.

Relying on the feature flag within the react component/page is insufficient, because unless these conditions are met we don't want the page to even load. (And not, try to load, but report an error or render a blank page instead.) Whereas having the check at the router-layer, provides the "regular" 404 page experience, doesn't require loading the additional JS bundle, etc.

Interesting! Also with slow network enabled? I would've thought that the initial loading state would be problematic here 🤔

🤷 No, just running locally. So it could be that things happen to be "so fast" I'm not seeing this failure. Or, perhaps it is totally broken, but I'm just not noticing because the page reloads to quickly or something?

But I 100% believe you that requiring some sort of async-data within the URL routing is a bad idea. So I'm happy to make this change.

unless these conditions are met we don't want the page to even load

@chrsmith Why is that? I don't see any harm done if we start loading the page and then show an error or a blank page while this feature is unannounced. Do you see a scenario where UX suffers because you move the check from the routing layer to the Page component?

Do you see a scenario where UX suffers because you move the check from the routing layer to the Page component?

Yes, this is very much impacts the user experience.

If the router doesn't recognize the URL path, then it serves a pretty nice 404 error page. If we pass that responsibility onto the component then all bets are off.

Maybe the component is built "correctly", and renders the same 404 page. So the UX is identical. Maybe it doesn't, so some 404 pages look weird. Or perhaps the component doesn't handle this well at all, and just throws an error.

Ultimately, if we remove the requirement here from the specific components (so it's one fewer thing they need to implement in order to be "correct"), and put that logic into the router. Then the overall system is easier to reason about and maintain.

It looks like the setClientSecret and setErrorDetails dependencies are missing from this useEffect call 🤔
Did you manage to run this? Maybe it only causes a linter error, I'm not sure, my IDE pretty much keeps this list auto-updated.

I think it's fine to leave setters out of there because they are known to be immutable. But that requires to know it's a setState setter so I bet that's why autocomplete for that won't consider it.

I believe I ran the linger locally, and saw this pass in the CI/CD. But regardless, I agree we should add these as explicit dependencies. (Even if they currently happen to be immutable... so unless the linter complains that the are added. I'll make the change.)

Nit: "Opts" is pretty Go-ish. "Options" is idiomatic.

Nit: I'd simplify this to: