handlebars-lang / handlebars.js

Minimal templating on steroids.
http://handlebarsjs.com
MIT License
17.82k stars 2.04k forks source link

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

Closed marnixk closed 1 year ago

marnixk commented 1 year ago

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 commented 1 year ago

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 */? :thinking:

marnixk commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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)

marnixk commented 1 year ago

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.

jaylinski commented 1 year ago

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