Skip to content

Conversation

@etriebe
Copy link
Contributor

@etriebe etriebe commented Aug 1, 2025

Haven't tested yet but just attempting to get this started.

@lahma
Copy link
Collaborator

lahma commented Aug 1, 2025

You can mark a PR as a draft so it won't be accidentally merged.

@lahma
Copy link
Collaborator

lahma commented Aug 1, 2025

Please add at least one test case that enables this option and does both VerifyHelper.Verify() and TypeScriptCompiler.AssertCompiles() there are examples around the TS test suite.

@etriebe etriebe marked this pull request as draft August 1, 2025 18:05
credentials: 'include',
{%- endif -%}
{%- if UseCorsMode -%}
mode: 'cors',
Copy link
Collaborator

@lahma lahma Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because there are four possible values: https://developer.mozilla.org/en-US/docs/Web/API/RequestInit#mode should this be an enum on settings side? I don't know about other clients if they support same options, but then it would be public enum CorsMode or FetchCorsMode with default of None, client template model could have Mode property which would map enum to string version

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the same applies to Credientials too, it has other options which would then be hard to set if it's hard-coded to include if credentials is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can change to be an enum that I assume would be nullable so people don't have to have it set?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well if it defaults to default of first value None I think there would be no need to set it? I have no strong opinions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lahma is there a way to use an enum equivalence check in the liquid files? I don't see any existing examples. I'm not sure what code options I have versus if I use a boolean I see prior art for me to do an if check.

Copy link
Contributor Author

@etriebe etriebe Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short of me just creating a unique boolean for each different enum option in NSwag.CodeGeneration.TypeScript\Models\TypeScriptClientTemplateModel.cs. Which feels hacky but would be relatively easy.

Copy link
Collaborator

@lahma lahma Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just add a string property that returns correct value, null should evaluate to false in if check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good call. Thanks

@lahma
Copy link
Collaborator

lahma commented Aug 2, 2025

Checked the PR and spec, some notes:

  • RequestCredentialsType probably doesn't need NotSet, as spec says that: Defaults to same-origin
  • RequestModeType probably doesn't need NotSet, as spec says that: Defaults to cors

Now it remains a question whether it needs to be rendered in the JS file if it is the default - your current logic incorrectly returns empty string, like I said, null needs to be returned.

@etriebe
Copy link
Contributor Author

etriebe commented Aug 4, 2025

I was thinking about having no NotSet but then it'd change a bunch of people's client files unnecessarily. I would generally say I'd want it to not appear in people's file and therefore I'd need to do one of two things

  1. Keep NotSet as an option for both and don't put anything in the file if it is there
  2. Regardless of if they set it themselves or it stays the default, if either of these two options are present we don't add anything to the ts file. This doesn't feel great because if people have that set they probably want to see it show up in the file.

@etriebe etriebe changed the title DO NOT MERGE: Add WithCredentials to fetch template generation Add credentials and mode attributes to fetch template generation Aug 4, 2025
@etriebe etriebe marked this pull request as ready for review August 4, 2025 04:09
@lahma
Copy link
Collaborator

lahma commented Aug 4, 2025

I was thinking about having no NotSet but then it'd change a bunch of people's client files unnecessarily. I would generally say I'd want it to not appear in people's file and therefore I'd need to do one of two things

  1. Keep NotSet as an option for both and don't put anything in the file if it is there
  2. Regardless of if they set it themselves or it stays the default, if either of these two options are present we don't add anything to the ts file. This doesn't feel great because if people have that set they probably want to see it show up in the file.

I think the NotSet will be the value if you save the nswag.json using NSwagStudio for example, so the property will pesisted there, if you don't want it to appear then it probably needs JsonProperty attribute to ignore the default value.

@lahma
Copy link
Collaborator

lahma commented Aug 18, 2025

@etriebe do you have time/plans to fix the current test problems? 🙏🏻

@etriebe
Copy link
Contributor Author

etriebe commented Aug 18, 2025

Let me take a look. My development desktop died the other week so it's been a minute to get setup and configured again.

@etriebe
Copy link
Contributor Author

etriebe commented Aug 18, 2025

@lahma I am running locally and even just running for one of my tests I see the following and it spins for a very long time. Is there some sort of setup I need to do to make this work?

========== Starting test run ==========
Connecting to client host '127.0.0.1' port '55825'

@lahma
Copy link
Collaborator

lahma commented Aug 18, 2025

@lahma I am running locally and even just running for one of my tests I see the following and it spins for a very long time. Is there some sort of setup I need to do to make this work?

========== Starting test run ========== Connecting to client host '127.0.0.1' port '55825'

Haven't seen this before, might be related to your reset setup?

@etriebe
Copy link
Contributor Author

etriebe commented Aug 18, 2025

I think tsc is failing to install for me and/or it isn't on my path variable. How do you install tsc?

@lahma
Copy link
Collaborator

lahma commented Aug 18, 2025

Try running build.cmd in repository root.

@etriebe
Copy link
Contributor Author

etriebe commented Aug 18, 2025

When I run build.cmd it fails and says it can't find msbuild. And I have VS 2022 installed.

PowerShell Desktop version 5.1.26100.4768
Microsoft (R) .NET SDK version 9.0.304
Unhandled exception. System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.InvalidOperationException: No instances of MSBuild could be detected.
Try calling RegisterInstance or RegisterMSBuildPath to manually register one.
   at Microsoft.Build.Locator.MSBuildLocator.RegisterDefaults()
   at Build..ctor() in D:\repos\NSwag\build\Build.cs:line 28
   at System.RuntimeType.CreateInstanceOfT()
   --- End of inner exception stack trace ---
   at System.RuntimeType.CreateInstanceOfT()
   at System.Activator.CreateInstance[T]()
   at Nuke.Common.Execution.BuildManager.Execute[T](Expression`1[] defaultTargetExpressions) in /_/source/Nuke.Build/Execution/BuildManager.cs:line 48
   at Nuke.Common.NukeBuild.Execute[T](Expression`1[] defaultTargetExpressions) in /_/source/Nuke.Build/NukeBuild.cs:line 78
   at Build.Main() in D:\repos\NSwag\build\Build.cs:line 44

@etriebe
Copy link
Contributor Author

etriebe commented Aug 18, 2025

I had previously just installed .NET 9 SDK but installing .NET 8 SDK allowed this to run, too.

@etriebe
Copy link
Contributor Author

etriebe commented Aug 19, 2025

OK tests are passing locally now. @lahma Can you approve to run them here?

Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition, thanks for iterating! As a later improvement, we probably want these also in NSwagStudio and nswag.json.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants