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

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

Conversation

wanglam
Copy link
Contributor

@wanglam wanglam commented Nov 22, 2022

Description

This PR mainly replaces 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(). According to the discussion in #1561 , this PR only deals with the parts that can be replaced quickly. Therefore, most of the modifications only involve simple function replacement. Another major change is to replace the url parts in test config with URL object for transmission. Most files are modified with a file based commit, so we could just delete this commit if this change isn't necessary.

Issues Resolved

#1561

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@wanglam wanglam marked this pull request as ready for review November 22, 2022 09:53
@wanglam wanglam requested a review from a team as a code owner November 22, 2022 09:53
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2022

Codecov Report

Merging #2910 (a846ba1) into main (de06344) will decrease coverage by 0.07%.
The diff coverage is 48.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #2910      +/-   ##
==========================================
- Coverage   66.45%   66.38%   -0.07%     
==========================================
  Files        3208     3208              
  Lines       61593    61616      +23     
  Branches     9502     9504       +2     
==========================================
- Hits        40932    40906      -26     
- Misses      18384    18425      +41     
- Partials     2277     2285       +8     
Flag Coverage Δ
Linux 66.38% <48.00%> (-0.02%) ⬇️
Windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s/osd-test/src/failed_tests_reporter/github_api.ts 7.84% <ø> (-1.78%) ⬇️
...st/src/functional_test_runner/lib/config/config.ts 43.90% <0.00%> (-2.05%) ⬇️
...st/src/functional_test_runner/lib/config/schema.ts 94.73% <ø> (ø)
...unner/lib/docker_servers/docker_servers_service.ts 27.94% <ø> (-1.05%) ⬇️
...egacy_opensearch/legacy_opensearch_test_cluster.js 20.45% <0.00%> (-1.77%) ⬇️
...st/src/legacy_opensearch/opensearch_test_config.js 15.78% <0.00%> (-10.88%) ⬇️
packages/osd-test/src/osd/osd_test_config.ts 10.00% <0.00%> (-5.79%) ⬇️
src/core/public/chrome/ui/header/header_logo.tsx 34.09% <0.00%> (ø)
src/core/public/chrome/ui/header/home_loader.tsx 17.14% <0.00%> (ø)
src/core/server/http/base_path_proxy_server.ts 6.66% <0.00%> (-0.32%) ⬇️
... and 12 more

... and 7 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@wanglam wanglam force-pushed the refactor-replace-url-parse-format-resolve-with-whatwg-url branch from 1eaf259 to 9e8818c Compare November 23, 2022 02:43
@wanglam wanglam force-pushed the refactor-replace-url-parse-format-resolve-with-whatwg-url branch from 32c4581 to a2d7108 Compare February 9, 2023 12:50
@wanglam
Copy link
Contributor Author

wanglam commented Feb 14, 2023

@joshuarrrr @AMoo-Miki I have pushed all the file changes. Feel free to help me review again. Thanks.

@joshuarrrr
Copy link
Member

Thanks! We'll try to see if we can find the capacity to review this week.

@joshuarrrr joshuarrrr changed the title [Refactor]Refactor replace url parse format resolve with whatwg url [Refactor] Replace url parse format resolve with whatwg url Feb 16, 2023
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

@wanglam Looking good?

A couple minor questions. The only major request I have left is to centralize the .toString().slice(0, -1) manipulation so that it's not peppered throughout the code.

opensearchDashboardsUrl = config
.get('servers.opensearchDashboards')
.fullURL.toString()
.slice(0, -1) as string;
Copy link
Member

Choose a reason for hiding this comment

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

do we still need the as string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. The config.get('servers.opensearchDashboards') will return an any object. The TS complier won't know its type. So I add the as string.

packages/osd-test/src/osd/osd_test_config.ts Show resolved Hide resolved
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

@wanglam Thanks for the perseverance! LGTM!

packages/osd-test/src/osd/osd_test_config.ts Show resolved Hide resolved
Copy link
Collaborator

@AMoo-Miki AMoo-Miki left a comment

Choose a reason for hiding this comment

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

There are some leftovers of the legacy URL APIs but we could deal with them incrementally after merging this since these changes are slated for v3.0.0.

@joshuarrrr
Copy link
Member

Yeah, here's the scope of this PR: #1561 (comment)

After merging, I'll create a follow-up issue for the trickier bits.

@joshuarrrr joshuarrrr merged commit 2de11ff into opensearch-project:main Mar 24, 2023
pjfitzgibbons pushed a commit to pjfitzgibbons/OpenSearch-Dashboards that referenced this pull request Mar 24, 2023
…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]>
sikhote pushed a commit to sikhote/OpenSearch-Dashboards that referenced this pull request Apr 24, 2023
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Chore] Update deprecated url methods (url.parse(), url.format())
4 participants