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: remove implicit global references #932

Merged
merged 3 commits into from
Apr 17, 2022
Merged

Conversation

ph-fritsche
Copy link
Member

What:

Remove global references or make them explicit.

Why:

There might be problems with shadowed global variables in testing environments. See #892
The way e.g. jest hoists code we cannot be sure that a reference to e.g. window in a module is a reference to globalThis.window. We further cannot be sure that e.g. document.defaultView references the same object as window.
Although this is probably no issue in most cases we should be explicit about where we access objects through variables the user provided per setup() and where we access through global variables.

#929 , #931 enforce to be explicit about access to globals. Any reference that can't be found per text search for globalThis. results in a linting error.

How:

  • Remove global references from Clipboard util. We already pass down window to the create functions so we can use the scoped variable in the class.
    We still access the global window for our afterEach and afterAll hooks to reset/detach the Clipboard stub. This is probably not an issue as the environments providing a global after* hook are probably identical to environments merging window into the global scope (like jest+jsdom).
    If you happen to use an environment where you need to work around this, please leave a comment.
  • Make global references in setup() explicit. If the user provides a document with defaultView, these won't be used.
  • Make global references (parseInt, setTimeout) in utils explicit.

Checklist:

  • Tests
  • Ready to be merged

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 17, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e7dc587:

Sandbox Source
userEvent-dom Configuration
userEvent-react Configuration

@codecov
Copy link

codecov bot commented Apr 17, 2022

Codecov Report

❗ No coverage uploaded for pull request base (globals-merge@31a808b). Click here to learn what that means.
The diff coverage is n/a.

@@                Coverage Diff                @@
##             globals-merge      #932   +/-   ##
=================================================
  Coverage                 ?   100.00%           
=================================================
  Files                    ?        85           
  Lines                    ?      1784           
  Branches                 ?       641           
=================================================
  Hits                     ?      1784           
  Misses                   ?         0           
  Partials                 ?         0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31a808b...e7dc587. Read the comment docs.

@ph-fritsche ph-fritsche changed the base branch from main to globals-merge April 17, 2022 08:43
@ph-fritsche ph-fritsche merged commit 9913798 into globals-merge Apr 17, 2022
@ph-fritsche ph-fritsche deleted the globals-fix branch April 17, 2022 08:48
@github-actions
Copy link

🎉 This PR is included in version 14.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

1 participant