Skip to content
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

global variable shadowed? #1026

Closed
dosentmatter opened this issue Sep 10, 2021 · 2 comments · Fixed by #1355
Closed

global variable shadowed? #1026

dosentmatter opened this issue Sep 10, 2021 · 2 comments · Fixed by #1355

Comments

@dosentmatter
Copy link

dosentmatter commented Sep 10, 2021

https://github.com/github/fetch/blob/d1d09fb8039b4b8c7f2f5d6c844ea72d8a3cefe6/fetch.js#L1-L5

I'm not sure if I'm reading this wrong, but it seems like the second global is shadowed by the var global declaration. After hoisting, the equivalent code is

var global; // default undefined
global =
  (typeof globalThis !== 'undefined' && globalThis) ||
  (typeof self !== 'undefined' && self) ||
  (typeof global !== 'undefined' && global) ||
  {}

Because of hoisting, the global gets evaluated as false in all environments:

(typeof global !== 'undefined' && global)
// becomes
(typeof undefined !== 'undefined' && global)
// becomes
('undefined' !== 'undefined' && undefined)
// becomes
(false &&  undefined)
// becomes
false

I think the intent was to access the global object, if it exists.

We might have to capture it to a variable, oldGlobal, first, but the hoisting of global prevents us from doing this easily.
For example, the following wouldn't work because oldGlobal and global declarations are hoisted.

var oldGlobal = global; // === undefined
var global =
  (typeof globalThis !== 'undefined' && globalThis) ||
  (typeof self !== 'undefined' && self) ||
  (typeof oldGlobal !== 'undefined' && oldGlobal) ||
  {}

The only solution I can think of that preserves the variable name, global, is to wrap the code in an IIFE to prevent hoisting above oldGlobal. The typeof global is also moved up to oldGlobal to prevent errors on environments that don't global defined.

var oldGlobal = typeof global !== 'undefined' && global
(function () {
  var global =
    (typeof globalThis !== 'undefined' && globalThis) ||
    (typeof self !== 'undefined' && self) ||
    oldGlobal ||
    {}

  // rest of the code...
})()

An alternative solution is to rename global to some other name.

@dosentmatter
Copy link
Author

dosentmatter commented Sep 10, 2021

Note that the behavior of var window or var global differ depending if you are in the global scope, module, or function scope.

Chrome console/snippet:
This is in the global scope and window is already declared so var window does nothing.

var window = window;
window; // still the global window
(function () { var window; return window; })(); // undefined

Node.js repl:
Also in the global scope.

var global = global;
global; // still the global object
(function () { var global; return global; })(); // undefined

Node.js file:
Module scope so var global shadows the global-scoped global.

var global = global;
global; // undefined
(function () { var global; return global; })(); // undefined

Notice when the code gets wrapped in a function, window/global is consistently undefined. The fetch code does get wrapped in a function if we are using a module bundler like webpack.

@oliverlj
Copy link

oliverlj commented May 3, 2022

I'm wondering if this is causing error I have.

 Failed to execute 'fetch' on 'Window': Failed to read the 'signal' property from 'RequestInit': Failed to convert value to 'AbortSignal'.

i saw this other project doing an import : Azure/azure-sdk-for-js#9880

JakeChampion added a commit that referenced this issue Jul 17, 2023
JakeChampion added a commit that referenced this issue Jul 17, 2023
cr313 added a commit to cr313/fetch-Js-flow that referenced this issue Apr 19, 2024
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 a pull request may close this issue.

4 participants
@oliverlj @dosentmatter and others