Skip to content

[typescript-fetch] check if window is undefined#3326

Closed
hello-brsd wants to merge 2 commits into
OpenAPITools:masterfrom
hello-brsd:support-node-fetch
Closed

[typescript-fetch] check if window is undefined#3326
hello-brsd wants to merge 2 commits into
OpenAPITools:masterfrom
hello-brsd:support-node-fetch

Conversation

@hello-brsd

Copy link
Copy Markdown
Contributor

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

The window reference should be checked, because if the client run on server side (universal js),window is maybe undefined and fetch can be in the global scope (for example node-fetch).

@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @nicokoenig (2018/09) @topce (2018/10) @akehir (2019/07)

@auto-labeler

auto-labeler Bot commented Jul 9, 2019

Copy link
Copy Markdown

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.


get fetchApi(): FetchAPI {
return this.configuration.fetchApi || window.fetch.bind(window);
return this.configuration.fetchApi || (typeof window !== 'undefined' ? window.fetch.bind(window) : fetch );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the symbol fetch isn't declared or defined anywhere in this file, so this should be a compilation error, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, fetch is available on global scope, if you have a shim in nodejs:

global.fetch = global.fetch || require('node-fetch')

Similarly to FormData:

global.FormData = global.FormData || require('form-data')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

but what if fetch is not defined in the global scope? or is it always available in the global scope? if so, why use window.fetch at all?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

according to https://developer.mozilla.org/de/docs/Web/API/WindowOrWorkerGlobalScope/fetch fetch should be available in any context

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it is really available in the global scope in every browser (checked in Firefox & Chromium) & in node, I'd be in favor of only checking fetch if `configuration.fetchApi' isn't defined, i.e.

return this.configuration.fetchApi || fetch; 

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@hello-brsd can you implement the change suggested by @TiFu?

@wing328

wing328 commented Jul 25, 2019

Copy link
Copy Markdown
Member

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@macjohnny

Copy link
Copy Markdown
Member

@hello-brsd any update here?

1 similar comment
@macjohnny

Copy link
Copy Markdown
Member

@hello-brsd any update here?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants