Skip to content

Fix node/no-deprecated-api issues#9670

Merged
whymarrh merged 1 commit intoMetaMask:developfrom
whymarrh:fix-node/no-deprecated-api
Oct 22, 2020
Merged

Fix node/no-deprecated-api issues#9670
whymarrh merged 1 commit intoMetaMask:developfrom
whymarrh:fix-node/no-deprecated-api

Conversation

@whymarrh
Copy link
Copy Markdown
Contributor

@whymarrh whymarrh commented Oct 21, 2020

Refs #9663

See node/no-deprecated-api for more information.

This change enables node/no-deprecated-api and fixes the issues raised by the rule.

The change to the way that punycode is imported is to workaround the fact that the 3rd-party version is hidden by the built-in version. Importing the file allows Node to understand that it should look in node_modules.

@whymarrh whymarrh force-pushed the fix-node/no-deprecated-api branch from 56f0ff3 to 0ade0e3 Compare October 21, 2020 20:00
@whymarrh whymarrh marked this pull request as ready for review October 21, 2020 20:20
@whymarrh whymarrh requested a review from a team as a code owner October 21, 2020 20:20
@whymarrh whymarrh requested a review from danfinlay October 21, 2020 20:20
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The WHATWG URL object doesn't contain a path property (see this comparison). This path property from the deprecated API is equivalent to ${pathname}{search}.

It looks like pathname is what was originally intended here. In attemptResolve below it is concatenated with search. This implies that if search was non-empty, we would see it duplicated on these resolved URLs 🤔

I will test this theory, and fix this in a separate PR if so.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have confirmed that this is a bug. It is addressed in #9674

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seeing searchParams used reminded me of this: https://twitter.com/domenic/status/1257377082704900096

I was curious, so I checked to see if there was any behaviour change here between url and URL. It looks as though there is not. url parses the query string in the same weird way that URL does, at least with the examples listed in that PR description anyway. I didn't try the linked exhaustive list. Of course it doesn't matter here for this mock server anyway.

Refs MetaMask#9663

See [`node/no-deprecated-api`][1] for more information.

This change enables `node/no-deprecated-api` and fixes the issues raised by the rule.

  [1]:https://github.com/mysticatea/eslint-plugin-node/blob/v11.1.0/docs/rules/no-deprecated-api.md

The change to the way that `punycode` is imported is to address the fact that
third-party module is hidden by the built-in. This is a silly hack but it works.
@whymarrh whymarrh force-pushed the fix-node/no-deprecated-api branch from 0ade0e3 to 01e938a Compare October 22, 2020 13:31
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@whymarrh whymarrh merged commit 362e717 into MetaMask:develop Oct 22, 2020
@whymarrh whymarrh deleted the fix-node/no-deprecated-api branch October 22, 2020 14:03
@github-actions github-actions bot locked and limited conversation to collaborators Oct 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants