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

chore(e2e): Lint for restricted globals in e2e tests #6600

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kraenhansen
Copy link
Contributor

Description

The WebdriverIO API allows for execution of serialized JavaScript functions in the browser via the browser.execute method. We're currently using this. To get typescript support for this, our tsconfig includes "dom" as a lib, which has the unfortunate drawback that the types for DOM globals become available to all the code, which is running on Node.js and doesn't have values for these available at runtime.

This is an issue first and foremost because the usefulness of TypeScript degrades fast when you cannot trust it and most recently when I wanted to use the fetch implementation built into Node.js, as the DOM's fetch declaration overlap and differ slightly.

TLDR; It's not trivial to restrict access to these types In situations like this, I've reached for a multi-project setup using TypeScript references in the past: They can specify subsets of files which are allowed access to types based on specific TypeScript configs.

This won't work in this particular situation however, because:

  1. The WebdriverIO's browser.execute is inlined with other code which is not allowed to access DOM APIs and the tsconfigs can't control types at the granularity of blocks of code. This could be worked around by extracting these functions into separate files with a known extension (such as whatever.dom.ts) which would indicate code which would be unrestricted from accessing DOM globals, but this does come with a drawback of decreased cohesion between the function and the code calling ti.
  2. TypeScript project references need project configs to use "composite", which entails either noEmit: false or emitOnlyDeclaration: true, meaning they will emit code, which is not ideal in our scenario, as we're using ts-node to strip types and interpret the code without an explicit build step.

My proposal for an interim solution is inspired by this comment, where the "globals" package is used in conjunction with the "no-restricted-globals" eslint rule to produce errors and warnings when accessing DOM-only globals.

Merging this PR will:

  • Enable eslint errors for code accessing DOM globals in general.
  • De-escalate to eslint warnings for files which we know have browser.execute calls.
  • Add eslint-ignore comments for existing use of these DOM globals, where I've verified the code is executed through browser.execute calls.

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@kraenhansen kraenhansen self-assigned this Jan 9, 2025
@kraenhansen kraenhansen force-pushed the kh/e2e-tests-dom-globals branch from ef00394 to cb84b7a Compare January 9, 2025 12:14
@kraenhansen kraenhansen added the no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion) label Jan 9, 2025
@kraenhansen
Copy link
Contributor Author

kraenhansen commented Jan 9, 2025

Check is currently failing because I'm using a feature introduced in Node v22, I'll revert that.

@kraenhansen kraenhansen force-pushed the kh/e2e-tests-dom-globals branch from cb84b7a to 5962368 Compare January 9, 2025 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant