[Chore] Update deprecated url methods (`url.parse()`, `url.format()`) #1561

joshuarrrr · 2022-05-06T18:40:16Z

These have been deprecated since node v11.0.0. The recommendation is to "Use the WHATWG URL API" instead.

Parsing can generally be done through the new URL() constructor pattern.

format() still exists, although the signature has changed, and it may no longer be needed in many places.

(via #1553 (comment))

The text was updated successfully, but these errors were encountered:

wanglam · 2022-11-02T02:45:15Z

@joshuarrrr I will take this issue, and may raise a PR about it later.

ashwin-pc · 2022-11-04T06:19:47Z

@wanglam can you add resolve to that list?

You can search for all imports from the url lib and see if we are using any other deprecated methods

wanglam · 2022-11-07T06:02:23Z

@ashwin-pc Sure, the resolve method is also be marked legacy. We can find all the usages and replace them.

wanglam · 2022-11-16T03:46:38Z

According to my research and testing, using whatwg URL cannot completely replace the use cases of node url related methods. They mainly have the following differences:

Pathname with origin like (http://localhost/foo) is valid in url.format but steal treat as pathname in whatwg-URL
url.parse can parse relative path like (/foo/bar?a=b) , whatwg-URL will throw error if not an valid URL
For no leading slash URL, url.parse will keep the origin pathname like (foo/bar), new URL will add a slash to the pathname part.
whatwg-URL dosen't support 'slashesDenoteHost' equal false in url.parse which means it will treat 'foo' as a hostname for '////foo/bar' rather then a part of pathname.
Port will be ignored for specific protocol and port in whatwg-URL toString method like (new URL(‘http://localhost:80’).toString() will output ‘http://localhost’)
url.format could format url without protocol slash like (mail:foo), whatwg-URL can’t remove the slashes.

Among the above differences, 1, 2 and 5 could be to solve by some tricky way. According to my survey in the code base, the legacy method used in most scenarios can be directly replaced. The following scenarios (just list a part of them) may need to be implemented in other ways:

for packages/osd-std/src/url.test.js, there is a test case to change http protocol to mail, and remove the slashes. It's not support in whatwg-URL.
for src/core/public/http/base_path.test.js, there is a test case named 'returns the path unchanged if it does not start with a slash'. The whatwg-URL will append a slash after url convert.

There are still many other issues caused by differences 3 / 4 / 6. Seems the whatwg-URL is more suitable for standard URL situation.

ashwin-pc · 2022-11-17T21:40:22Z

@wanglam thanks for a very good deepdive into the issue. I think you have called out the risks accurately here. the whatwg-url does not appear to be ready as a drop in replacement for all our url.parse and url.format use cases. @AMoo-Miki @kavilla @joshuarrrr what are your thoughts on his findings?

Whatwg-url was introduced to standardize browser url's but has some issues when used in node environments. There are open issues in other prominent repo's that cannot deprecate their uses of parse and format because of it. My personal opinion here is to only replace the instances where a drop in replacement does not affect expected functionality and leave comments referencing this issue for the ones that we cannot.

joshuarrrr · 2022-11-18T00:25:34Z

My personal opinion here is to only replace the instances where a drop in replacement does not affect expected functionality and leave comments referencing this issue for the ones that we cannot.

+1. We should definitely first focus on the obvious/easy low-hanging fruit. If we could remove like 90% of the deprecated usage that would be great.

wanglam · 2022-11-22T09:24:38Z

@joshuarrrr @ashwin-pc I create a PR(#2910 ) about this issue. Could you help me to review it?

joshuarrrr · 2022-11-23T00:01:35Z

Thanks @wanglam! We'll schedule it for review. With the holiday week it may realistically be next week that we can pick it up.

AMoo-Miki · 2022-12-06T18:29:23Z

Just a side note: url is not deprecated; it is just marked as legacy:

Stability: 3 - Legacy. The feature is no longer recommended for use. While it likely will not be removed, and is still covered by semantic-versioning guarantees, use of the feature should be avoided.

There is no urgency to getting rid of url.

Replace the url.parse/url.format/url.resolve function in the code base with whatwg-url. The main change is to change the url.parse to new URL(), and change url.format to [URL object].toString and change the url.resolve to new URL() toString(). Another major change is to replace the url parts in test config with URL object for transmission. Partially completes #1561 Signed-off-by: Lin Wang <[email protected]> Co-authored-by: Josh Romero <[email protected]>

joshuarrrr · 2023-03-24T18:16:31Z

#2910 fixed much of the low hanging fruit. See #1561 (comment) for remaining tasks.

…ch-project#2910) Replace the url.parse/url.format/url.resolve function in the code base with whatwg-url. The main change is to change the url.parse to new URL(), and change url.format to [URL object].toString and change the url.resolve to new URL() toString(). Another major change is to replace the url parts in test config with URL object for transmission. Partially completes opensearch-project#1561 Signed-off-by: Lin Wang <[email protected]> Co-authored-by: Josh Romero <[email protected]>

Aigerim-ai · 2023-04-08T15:39:38Z

Hello! is it possible to create issues out of remaining tasks, I am not exactly sure what is our remaining? I think we need more clarity for this issue to be closed.

As I saw from https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2910/files
some code is not merged into main

For example we still have:

if (!opensearchUrl && config) {
opensearchUrl = Url.format(config.get('servers.opensearch'));
}

but in: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2910/files the code was replace with:

if (!opensearchUrl && config) {
opensearchUrl = config.get('servers.opensearch.serverUrl');
}

Could you please clarify on this part?

Thank you in advance!

wanglam · 2023-04-10T02:30:15Z

Hi @Aigerim-ai , have you update your forked repository main branch? Seems the merged commit wasn't exists in your branch. I can't find code like Url.format in the main branch code.

I have searched all the from 'url' in the latest main branch, as shown below

The remaining files need to deal with the relative URL. For now, the URL object is hard to process this part.

…ch-project#2910) Replace the url.parse/url.format/url.resolve function in the code base with whatwg-url. The main change is to change the url.parse to new URL(), and change url.format to [URL object].toString and change the url.resolve to new URL() toString(). Another major change is to replace the url parts in test config with URL object for transmission. Partially completes opensearch-project#1561 Signed-off-by: Lin Wang <[email protected]> Co-authored-by: Josh Romero <[email protected]> Signed-off-by: David Sinclair <[email protected]>

joshuarrrr mentioned this issue May 6, 2022

Chore/migrate mocha tests #1553

Merged

7 tasks

joshuarrrr added good first issue Good for newcomers dependencies Pull requests that update a dependency file labels May 6, 2022

tmarkley added the technical debt If not paid, jeapardizes long-term success and maintainability of the repository. label May 25, 2022

joshuarrrr added the hacktoberfest Global event that encourages people to contribute to open-source. label Oct 4, 2022

wanglam mentioned this issue Nov 22, 2022

[Refactor] Replace url parse format resolve with whatwg url #2910

Merged

8 tasks

kavilla linked a pull request Dec 2, 2022 that will close this issue

[Refactor] Replace url parse format resolve with whatwg url #2910

Merged

8 tasks

joshuarrrr closed this as completed in #2910 Mar 24, 2023

joshuarrrr reopened this Mar 24, 2023

github-actions bot added the untriaged label Mar 24, 2023

joshuarrrr added help wanted Community development is encouraged and removed untriaged labels Mar 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Chore] Update deprecated url methods (`url.parse()`, `url.format()`) #1561

[Chore] Update deprecated url methods (`url.parse()`, `url.format()`) #1561

joshuarrrr commented May 6, 2022

wanglam commented Nov 2, 2022

ashwin-pc commented Nov 4, 2022

wanglam commented Nov 7, 2022 •

edited

Loading

wanglam commented Nov 16, 2022 •

edited

Loading

ashwin-pc commented Nov 17, 2022

joshuarrrr commented Nov 18, 2022

wanglam commented Nov 22, 2022

joshuarrrr commented Nov 23, 2022

AMoo-Miki commented Dec 6, 2022

joshuarrrr commented Mar 24, 2023

Aigerim-ai commented Apr 8, 2023

wanglam commented Apr 10, 2023

[Chore] Update deprecated url methods (url.parse(), url.format()) #1561

[Chore] Update deprecated url methods (url.parse(), url.format()) #1561

Comments

joshuarrrr commented May 6, 2022

wanglam commented Nov 2, 2022

ashwin-pc commented Nov 4, 2022

wanglam commented Nov 7, 2022 • edited Loading

wanglam commented Nov 16, 2022 • edited Loading

ashwin-pc commented Nov 17, 2022

joshuarrrr commented Nov 18, 2022

wanglam commented Nov 22, 2022

joshuarrrr commented Nov 23, 2022

AMoo-Miki commented Dec 6, 2022

joshuarrrr commented Mar 24, 2023

Aigerim-ai commented Apr 8, 2023

wanglam commented Apr 10, 2023

[Chore] Update deprecated url methods (`url.parse()`, `url.format()`) #1561

[Chore] Update deprecated url methods (`url.parse()`, `url.format()`) #1561

wanglam commented Nov 7, 2022 •

edited

Loading

wanglam commented Nov 16, 2022 •

edited

Loading