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

Calypso Runtime Error in WP-Desktop: “globalThis is not defined” #38819

Closed
nsakaimbo opened this issue Jan 14, 2020 · 7 comments · Fixed by Automattic/wp-desktop#743
Closed
Labels
[Feature] WordPress Desktop App Features and improvements related to the WordPress Desktop App. [Pri] BLOCKER

Comments

@nsakaimbo
Copy link
Contributor

Description

A recent change to Calypso has introduced a runtime error in WP-Desktop as of latest Calypso master (652ca9a).

Steps to Reproduce:

  1. Build wp-desktop with latest Calypso master
  2. Attempt to run the application (on Mac, double-click the built WordPress.com desktop app in the release/mac directory)
  3. The application will display an error message like the following prior to shutting down:
ReferenceError: globalThis is not defined
    at Object.module.exports.Object.defineProperty.value (/Users/nsakaimbo/workspace/automattic/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/build/desktop.js:39:4557)
    at n (/Users/nsakaimbo/workspace/automattic/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/build/desktop.js:1:217)
    at Object.module.exports.Object.defineProperty.value (/Users/nsakaimbo/workspace/automattic/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/build/desktop.js:531:276948)
    at n (/Users/nsakaimbo/workspace/automattic/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/build/desktop.js:1:217)
    at Object.<anonymous> (/Users/nsakaimbo/workspace/automattic/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/build/desktop.js:531:275577)
    at n (/Users/nsakaimbo/workspace/automattic/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/build/desktop.js:1:217)
    at Object.module.exports.Object.defineProperty.value (/Users/nsakaimbo/workspace/automattic/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/build/desktop.js:531:262480)
    at n (/Users/nsakaimbo/workspace/automattic/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/build/desktop.js:1:217)
    at Object.module.exports.Object.defineProperty.value (/Users/nsakaimbo/workspace/automattic/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/build/desktop.js:531:250492)
    at n (/Users/nsakaimbo/workspace/automattic/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/build/desktop.js:1:217)
// ...

Screen Shot 2020-01-13 at 7 18 27 PM

Notes: The last known good SHA of Calypso in WP-Desktop is b288baa, and I first encountered this error when trying to merge a838790, indicating the breaking change was introduced into Calypso somewhere between those commits.

@nsakaimbo nsakaimbo added [Pri] BLOCKER [Feature] WordPress Desktop App Features and improvements related to the WordPress Desktop App. labels Jan 14, 2020
@nsakaimbo nsakaimbo changed the title Calypso Runtime Error in WP-Desktop Calypso Runtime Error in WP-Desktop: “globalThis is not defined” Jan 14, 2020
@jsnajdr
Copy link
Member

jsnajdr commented Jan 14, 2020

I think it was broken in #38560 by @sgomes, as he added a usage of globalThis.fetch.

The problem is that globalThis is not polyfilled in the desktop app. We'll have a very nice fix once #38660 is merged and the @automattic/calypso-polyfills package is published. Then we can add

require( '@automattic/calypso-polyfills' );

at the beginning of desktop/index.js

The fastest hotfix right now should be to define the globalThis global at the beginning of desktop/index.js:

globalThis = global;

@nsakaimbo
Copy link
Contributor Author

nsakaimbo commented Jan 14, 2020

@jsnajdr I just tried the above fix and that didn't seem to work. 🤔 Added globalThis to desktop/index.js so it looks like:

const globalThis = global;
/**
 * Internal dependencies
 */
require( './app' )();

This blog post seems to indicate that polyfilling globalThis may be a little more involved? Seems there are some recent plugins that might be worth trying.

@sgomes
Copy link
Contributor

sgomes commented Jan 14, 2020

@jsnajdr I just tried the above fix and that didn't seem to work. 🤔 Added globalThis to desktop/index.js so it looks like:

const globalThis = global;
/**
 * Internal dependencies
 */
require( './app' )();

This blog post seems to indicate that polyfilling globalThis may be a little more involved? Seems there are some recent plugins that might be worth trying.

const globalThis = global; will not work, as it creates a new constant that's local to the current module. You need to create a global globalThis object, available across every module. For this, you'd need to remove the const from your code and use what @jsnajdr suggested verbatim, or use the following instead: global.globalThis = global. Which is super ugly 😄

A full polyfill is indeed more complex, but it covers a lot more details.

FWIW, I intend to merge #38660 tomorrow, so if you're ok with waiting until then you can implement the cleaner solution that @jsnajdr mentioned first! 👍

@nsakaimbo
Copy link
Contributor Author

Thanks for the pointer and the education, @sgomes! Totally fine to wait until the cleaner approach is merged and we can test that out instead.

@sgomes
Copy link
Contributor

sgomes commented Jan 15, 2020

Update: this has now been merged, and I believe @jsnajdr is looking into the desktop fix.

@jsnajdr
Copy link
Member

jsnajdr commented Jan 15, 2020

Yes, we need to publish the @automattic/calypso-polyfills package to npm and then use it in wp-desktop.

@nsakaimbo
Copy link
Contributor Author

The change in wp-desktop #743 looks good. We should verify that latest Calypso can build with wp-desktop after that PR lands before closing this issue out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] WordPress Desktop App Features and improvements related to the WordPress Desktop App. [Pri] BLOCKER
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants