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

Fix for when global and window object both not found. #1894

Merged
merged 1 commit into from
Oct 16, 2022

Conversation

marnixk
Copy link

@marnixk marnixk commented Oct 8, 2022

When using Handlebars in a Cloudflare Worker, an environment in which the window and global objects both don't exist, an error is thrown about window being undefined.

In this PR I have slightly changed the code so that if both window and global do not exist, a Handlebars.noConflict function is generated that just returns the original Handlebars instance.

@jaylinski
Copy link
Member

Hm... can we somehow check for not relying on global/window via eslint so we won't break it again in the future?

Maybe by removing the line /* global global, window */? 🤔

@marnixk
Copy link
Author

marnixk commented Oct 9, 2022

Hi, thanks for the feedback!

When I remove the comment with global variable declarations, eslint starts to complain about the use of global and window in the return statements.

I think we could probably leave it in here, it's only a local declaration anyway which means their usage won't impact the any potential uses in other places (although I haven't found any other uses of window)

Thanks!
-Marnix

@marnixk
Copy link
Author

marnixk commented Oct 12, 2022

Hey! I'm uncertain if you're waiting for some more input from me, or if the PR will be able to move through whatever processes are in place to get it merged at some point. Happy to provide anything you need!

Cheers! Marnix

@jaylinski
Copy link
Member

@marnixk I was still thinking about ways to test this, but until we ship native ES-module support, we can't test in web-workers.

I read a little bit about how the get the root-object. I'd prefer to use an "official" solution, like the one from https://mathiasbynens.be/notes/globalthis.

/* global globalThis */
export default function(Handlebars) {
  /* istanbul ignore next */
  // https://mathiasbynens.be/notes/globalthis
  (function() {
    if (typeof globalThis === 'object') return;
    Object.prototype.__defineGetter__('__magic__', function() {
      return this;
    });
    __magic__.globalThis = __magic__; // eslint-disable-line no-undef
    delete Object.prototype.__magic__;
  }());

  const $Handlebars = globalThis.Handlebars;

  /* istanbul ignore next */
  Handlebars.noConflict = function() {
    if (globalThis.Handlebars === Handlebars) {
      globalThis.Handlebars = $Handlebars;
    }
    return Handlebars;
  };
}

Can you verify if this works in a Cloudflare worker? (I think it should)

@jaylinski jaylinski mentioned this pull request Oct 14, 2022
8 tasks
@marnixk
Copy link
Author

marnixk commented Oct 16, 2022

That's a fun bit of code!

I've pushed your suggestion to the PR and tested it with my Cloudflare Worker -- it all seems to work great!

Thanks for the suggestions.

When using Handlebars in a Cloudflare Worker, an environment
in which the `window` and `global` objects both don't exist,
an error is thrown about `window` being undefined.

According to the ECMA specification, only `self` is available
in a worker. Since we support old runtimes in our 4.x branch,
we can't use `globalThis` but have to use a polyfill.
@jaylinski
Copy link
Member

Thanks! After #1864 is merged, I will also fix this in master (here we can just globalThis because we target Node >= 12).

@jaylinski jaylinski merged commit 3d3796c into handlebars-lang:4.x Oct 16, 2022
jaylinski added a commit that referenced this pull request Oct 19, 2022
This is needed in order to use `globalThis`, see #1894.
jaylinski added a commit that referenced this pull request Oct 19, 2022
Pulled from 4.x branch, see #1894.
jaylinski added a commit that referenced this pull request Oct 19, 2022
This is needed in order to use `globalThis`, see #1894.
jaylinski added a commit that referenced this pull request Oct 19, 2022
Pulled from 4.x branch, see #1894.
jaylinski added a commit that referenced this pull request Oct 19, 2022
This is needed in order to use `globalThis`, see #1894.
It also made it possible to remove some old polyfills and fallbacks.
jaylinski added a commit that referenced this pull request Oct 19, 2022
Pulled from 4.x branch, see #1894.
jaylinski added a commit that referenced this pull request Oct 29, 2022
This is needed in order to use `globalThis`, see #1894.
It also made it possible to remove some old polyfills and fallbacks.
jaylinski added a commit that referenced this pull request Oct 29, 2022
Pulled from 4.x branch, see #1894.
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 this pull request may close these issues.

2 participants