-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
[typescript-axios] Conditionally set user-agent #20571
[typescript-axios] Conditionally set user-agent #20571
Conversation
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.
thanks for the fix!
modules/openapi-generator/src/main/resources/typescript-axios/configuration.mustache
Show resolved
Hide resolved
The change in OpenAPITools#20067 has caused some issues with clients which run in a Browser. This commit replaces that change, leaving the default User-Agent for axios unmodified, and only sets the User-Agent if the `http-user-agent` parameter is provided during generation time.
b834203
to
0869010
Compare
cc |
I don't think this was the correct solution to the problem here. This PR removes the concept of a 'default' User-Agent header. The usage of the That issue doesn't seem to have much traction behind it, so we may need to implement a workaround, but I do think we should still attempt to give this generator a default header so it behaves like the others. Is there a different javascript/typescript generator that might have an example of some logic to not set this header if chrome/safari is detected? |
@ckoegel but the user agent can just be set by the user of the generated code or at code generation with the flag, right? |
Yes, but there is no default value any more, it was my understanding that the SDK should set that user agent header automatically, and then it can be changed either in the generated code or with this flag, is that not the case? |
i think its safer without a default value, especially for frontend clients |
i think its better to keep the template code simple and have the consumers provide a user agent string via the param instead of setting skipDefaultUserAgent=false, since the consumer would need to add some extra config either way |
I think the code will be simple regardless, #20611 adds 2 lines to the template file, and #20067 was already accepted. If we allow for both, backend client users can leave the config unchanged to get the default header, or override the default by setting This obviously isn't true for all users, but anyone wanting the user agent header to stay consistent with the package version would need to manually create the user agent header string based on the current package version, and then pass that in as a cli argument or update the config file each time the client is generated. This isn't as desirable as just updating one argument statically, in the case of a frontend user needing to add the IMO, the root of this issue is caused by an axios bug, and if that bug gets fixed, I don't think this generator should behave differently because it once existed. |
@macjohnny Is this worth investigating? I'm happy to open a PR with the changes I'm suggesting so it can be tested. |
not sure if its just an axios bug or would apply to browser clients in general. i still think at least for frontend clients, the default should be no user agent set. if its important for backend clients to set a user agent, they can do so with the parameter. @joscha whats your opinion? |
I agree with @ckoegel to set a default header. It will help developers of APIs to find out a culprit if issues happen in the way requests are made. Especially in regards to different versions. If someone wants to hide and/or adapt the user agent, they can override it and/or empty it. I believe the default should follow |
@ckoegel can we make a distinction between FE and BE clients, such that for FE clients by default the user agent will not be set? |
You mean when a client is used within a browser environment by a user or based on the language? |
ok so what i just tried is
in chrome, and that doesn't give me warnings, it just ignores the user-agent header and overrides it with the one set by chrome. |
@macjohnny @joscha Let me know if there's an action item that comes from this, I'd be happy to implement/contribute! |
The change in #20067 has caused some issues with clients which run in a Browser. This commit replaces that change, leaving the default User-Agent for axios unmodified, and only sets the User-Agent if the
http-user-agent
parameter is provided during generation time.PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master
(upcoming7.x.0
minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)@macjohnny