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

strict mode compatibility #1558

Merged
merged 1 commit into from
Mar 4, 2021
Merged

strict mode compatibility #1558

merged 1 commit into from
Mar 4, 2021

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Feb 26, 2021

This fixes #1557 by adopting globalThis, and polyfilling it for environments that don't implement it yet via @ungap/global-this.

Copy link
Contributor

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, we need to avoid poly filling in tests only. This PR as it stands could enable your tests to pass in circumstances that would not work when QUnit itself wasn't loaded. For example, imagine your application code could access globalThis but forgot to bring the polyfill along with it. With the current implementation in this PR your tests would pass still, but if you deployed to environments without a naturally available globalThis your application would crash.

Since the code for the polyfill is sooo small, it might just be easier to put it alongside our promise polyfill instead of using @ungap/global-this (which mutates the environment to define globalThis).

I think you could get away with something like this in lib/global-this-polyfill.js, then import that throughout:

/*
https://github.com/ungap/global-this/blob/v0.4.4/esm/index.js

Copyright (c) 2020, Andrea Giammarchi, @WebReflection

Permission to use, copy, modify, and/or distribute this software for any
purpose with or without fee is hereby granted, provided that the above
copyright notice and this permission notice appear in all copies.

THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH
REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT,
INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
PERFORMANCE OF THIS SOFTWARE.

-------

Patches for use in QUnit:

- 2021-02-25: Export as module only, don't change global scope as QUnit must not
  affect the host context (e.g. people may test their application intentionally
  with different or no polyfills and we must not affect that).

*/

let foundGlobalThis;

(function (Object) {
  if (typeof globalThis === "object") {
    foundGlobalThis = globalThis;
  } else {
    this
      ? get()
      : (Object.defineProperty(Object.prototype, "_T_", {
          configurable: true,
          get: get,
        }),
        _T_);

    function get() {
      foundGlobalThis = this || self;
      delete Object.prototype._T_;
    }
  }
})(Object);

export default foundGlobalThis;

@ef4 ef4 force-pushed the strict-mode branch 2 times, most recently from c5f6720 to 6334fd9 Compare February 26, 2021 15:08
This fixes qunitjs#1557 by adopting [globalThis](https://github.com/tc39/proposal-global), and polyfilling it for environments that don't implement it yet via `@ungap/global-this`.
@ef4
Copy link
Contributor Author

ef4 commented Feb 26, 2021

This now incorporates Robert's feedback to no longer leak the polyfill.

@rwjblue
Copy link
Contributor

rwjblue commented Mar 4, 2021

@Krinkle - Any chance you could review? The strict mode breakage was introduced in 2671d3f (moving to import global from 'global') due to the changes made in 484d396 (specifically the fake global module added in the rollup.config.js).

@Krinkle Krinkle merged commit eeaa482 into qunitjs:master Mar 4, 2021
@Krinkle
Copy link
Member

Krinkle commented Mar 4, 2021

Tested locally in Firefox, and in IE9-11 via BrowserStack.

@Krinkle Krinkle mentioned this pull request Mar 4, 2021
@rwjblue
Copy link
Contributor

rwjblue commented Mar 4, 2021

Thank you @Krinkle!

@22a
Copy link

22a commented Mar 11, 2021

Thanks for this fix! Is there any ETA on when it will be bundled into an npm release?

@Krinkle
Copy link
Member

Krinkle commented Mar 11, 2021

@22a Tomorrow, or else in the weekend.

@22a
Copy link

22a commented Mar 11, 2021

@Krinkle Wonderful, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Not strict mode safe
4 participants