Visitar URL original
fix: prevent hoisting of the undefined `global` variable in `browser.js` by valeriangalliat · Pull Request #1534 · node-fetch/node-fetch · GitHub
Skip to content

fix: prevent hoisting of the undefined global variable in browser.js - #1534

Merged
jimmywarting merged 1 commit into
node-fetch:2.xfrom
valeriangalliat:global-object-hoisting
Nov 8, 2022
Merged

jimmywarting merged 1 commit into
node-fetch:2.xfrom
valeriangalliat:global-object-hoisting

Conversation

@valeriangalliat

Copy link
Copy Markdown

Purpose

Because of JS hoisting var global to the top of the file, typeof global in getGlobal() will always be undefined.

This can be a problem when bundling this code and running it in an environment that doesn't have self of window defined but has global.

In practice I ran into this issue after browserifying node-fetch and running the bundle in node --experimental-fetch (just to test).

I can confirm after this fix I can browserify node-fetch and have it expose the global fetch function when run into node --experimental-fetch.

Changes

By using a different variable name like globalObject, we are able to read the "real" typeof global and get access to the global object that way.

@valeriangalliat valeriangalliat changed the title Prevent hoisting of the undefined global variable in browser.js fix: prevent hoisting of the undefined global variable in browser.js Apr 12, 2022
Because of JS hoisting `var global` to the top of the file, `typeof
global` in `getGlobal()` will always be `undefined`.

By using a different variable name like `globalObject`, we are able to
read the "real" `typeof global` and get access to the global object that
way.
@ahaoboy

ahaoboy commented Nov 8, 2022

Copy link
Copy Markdown

@valeriangalliat This global var is also need to change? or add var global = globalObject?

fetch.Promise = global.Promise;

 fetch.Promise = global.Promise;

@ahaoboy

ahaoboy commented Nov 8, 2022

Copy link
Copy Markdown

@LinusU @NotMoni @jimmywarting PTAL

@LinusU LinusU left a comment

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.

👍

@jimmywarting
jimmywarting merged commit 8bb6e31 into node-fetch:2.x Nov 8, 2022
@valeriangalliat
valeriangalliat deleted the global-object-hoisting branch December 22, 2022 19:43
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 2.6.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jimmywarting

Copy link
Copy Markdown
Collaborator

in some aftermath i think we should instead have used the globalThis instead but maybe to late now...

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