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.

Sign up for GitHub

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

Jump to bottom

Change AbortController to default import #579

Merged
merged 2 commits into from
Sep 1, 2021
Merged

Change AbortController to default import #579

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
659d374
Change AbortController to default import
adonesky1 Sep 1, 2021
12c8501
Update src/assets/TokenListController.ts
rekmarks Sep 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/assets/TokenListController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import contractMap from '@metamask/contract-metadata';
import type { Patch } from 'immer';
import { Mutex } from 'async-mutex';
import { AbortController } from 'abort-controller';
// eslint-disable-next-line
rekmarks marked this conversation as resolved.
Show resolved Hide resolved
import AbortController from 'abort-controller';
Copy link
Member

Choose a reason for hiding this comment

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

Let's use a // eslint-disable-next-line directive to avoid modifying the ESLint config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Can do. Just wondering though is that a lint rule we want to preserve? It feels like we may have opted into it by default?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we probably did, but the defaults are generally reasonable, and as a matter of expediency discussions about which lint rules we should use should be had on our ESLint config repo. For the purposes of this PR, I think it's best if we only concern ourselves about how we disable this particular rule for this particular instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair! Will update!

Copy link
Member

Choose a reason for hiding this comment

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

Although, now that we're here, I think that rule is good, because consider a module with both named and default exports. Many such packages exist, and the named and default exports are usually different. If you import the default export under the name of a named export, you find yourself in an annoying place if you ever also want to use the named export, since one of the names have to change. That seems bad to me!

This polyfill package is an exception in that the default and named export are the same thing, and IMO we should call them both AbortController.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I've swapped out the config change for a eslint-disable-next-line

import { BaseController } from '../BaseControllerV2';
import type { RestrictedControllerMessenger } from '../ControllerMessenger';
import { safelyExecute } from '../util';
Expand Down