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

support early returns and typeof checks for feature detection #596

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

bschlenk
Copy link

Hey Amila, thanks for building this plugin!

Sorry for the large PR, I'm hoping this will spark some discussion mainly, the code here is still WIP. But I wanted to get it in front of you before I get too far along.

As a lot of the issues point out, if statements prevent this plugin from detecting browser compat issues, and I wanted to take a shot at fixing it. But I wound up taking a few steps more and fixed a few other things.

Fundamentally this sort of changes how this plugin is implemented. But I think it makes sense as far as what this plugin is meant to catch. When I think about these apis, I don't really care whether they're function calls, new statements, or member expressions. The error is simply that the object/method doesn't exist in certain browsers, and that's what we want to catch.

With that in mind, this PR changes the approach to only lint on Identifier nodes. And it focuses on what types of operations would cause runtime errors. So any code that is testing if a feature exists is now okay:

if (window.fetch) {
  fetch()
}

if (!window.fetch) return;
fetch()

if (typeof fetch === "undefined") return;
fetch()

if (typeof fetch === "function") {
  fetch()
}

But if you do something that would result in a ReferenceError, we'll report it:

// this would throw a ReferenceError so it gets reported
if (fetch) {
  fetch()
}

I did a bit of other cleanup too, if that's desirable:

  • remove unnecessary types that TypeScript can infer automatically
  • use the types from eslint rather than redefining them
  • fixed the sorting of browser targets so they print out in alphabetical order, instead of reverse order

I'm happy to make whatever changes you'd like! I very much want to use this plugin, and these issues were the main blockers for me. I also plan on adding linting for unsupported regex features, and probably going through the list of other issues as well.

fixes:

@bschlenk bschlenk force-pushed the bschlenk/early-return branch from f608c8f to 71a7c36 Compare September 25, 2023 02:12
@bschlenk bschlenk force-pushed the bschlenk/early-return branch from 71a7c36 to ce1c17f Compare October 13, 2023 18:42
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.

1 participant