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

fix: remove deprecated usage of parse #1959

Merged
merged 3 commits into from
Mar 17, 2022

Conversation

jbl428
Copy link
Contributor

@jbl428 jbl428 commented Dec 4, 2021

Summary

Replace depreacted parse function to WHATWG URL.

Checklist

  • The basics
    • I tested these changes manually in my local or dev environment
  • Tests
    • Added or updated
    • N/A
  • Event Tracking
    • I added event tracking and followed the event tracking guidelines
    • N/A
  • Error Reporting
    • I reported errors and followed the error reporting guidelines
    • N/A

@jbl428 jbl428 requested review from a team and Amjcraft and removed request for a team December 4, 2021 14:32
@Nezteb Nezteb requested review from a team and brendarearden and removed request for a team March 2, 2022 15:48
@Nezteb
Copy link

Nezteb commented Mar 2, 2022

FYI @chohmann I added TC as a reviewer and removed @Amjcraft. This PR is old/stale, so I just wanted to make sure it's on your radar! (Aaron: If you still are willing/able to review this, feel free!)

@Nezteb Nezteb removed the request for review from Amjcraft March 2, 2022 15:50
@daniel-white daniel-white self-requested a review March 2, 2022 17:40
@@ -42,11 +42,11 @@ describe('User Http Client', () => {
jest.spyOn(client, 'request');
});

afterAll(() => jest.clearAllMocks());
Copy link
Member

Choose a reason for hiding this comment

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

why was this moved into the inner scopes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that previous mock's call history should be cleared before each describe started .

If second describe pass, the third one always pass because both have same test body.

      describe('when calling a method with overridden url', () => {
        beforeAll(() => client.get('/pet', { baseUrl: 'https://www.google.it' }));

        afterAll(() => jest.clearAllMocks());

        test('should call the mocker with the replaced full url', () => {
          expect(mock.default).toBeCalledWith({ // <-- same body
            resource: expect.anything(),
            input: expect.objectContaining({
              data: expect.objectContaining({
                url: expect.objectContaining({
                  baseUrl: 'https://www.google.it',
                  path: '/pet',
                }),
              }),
            }),
            config: expect.anything(),
          });
        });
      });

      describe('when calling a method with a full url', () => {
        beforeAll(() => client.get('https://www.google.it/pet'));

        afterAll(() => jest.clearAllMocks());

        test('should call the mocker with the replaced full url', () => {
          expect(mock.default).toBeCalledWith({  // <-- same body
            resource: expect.anything(),
            input: expect.objectContaining({
              data: expect.objectContaining({
                url: expect.objectContaining({
                  baseUrl: 'https://www.google.it',
                  path: '/pet',
                }),
              }),
            }),
            config: expect.anything(),
          });
        });
      });
    })

path: parsedUrl.pathname,
query: parseQueryString(parsedUrl.query || '') as IHttpNameValues,
query: parseQueryString(parsedUrl.search.substring(1)) as IHttpNameValues,
Copy link
Member

Choose a reason for hiding this comment

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

is search guaranteed not to be undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I found that it is always string even though there are no query string in URL.
but I think it is better to make code more defensive 😄

@@ -4,11 +4,12 @@ import { assertResolvesRight, assertResolvesLeft } from '@stoplight/prism-core/s
import { keyBy, mapValues } from 'lodash';
import { hopByHopHeaders } from '../resources';
import { DiagnosticSeverity, Dictionary } from '@stoplight/types';
import { format } from 'url';
Copy link
Member

Choose a reason for hiding this comment

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

i'm not seeing any functional changes here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is my mistake :)

const url = format({
...partialUrl,
host: partialUrl.host,
Copy link
Member

Choose a reason for hiding this comment

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

why include this if is already spread in from line 46?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since partialUrl.host is a getter not property. It is not copied properly.

Come to think of it, it would be better to use Object.assign instead of spread. :)

@jbl428 jbl428 requested a review from a team as a code owner March 3, 2022 14:10
@jbl428 jbl428 force-pushed the fix/chage-parse-url branch 2 times, most recently from 1980973 to 53a45ae Compare March 3, 2022 14:39
docs/guides/01-mocking.md Outdated Show resolved Hide resolved
@chohmann
Copy link
Contributor

The code looks good! Can you resolve the docs conflicts? We'll be good to go then!

@jbl428 jbl428 requested review from daniel-white and removed request for a team March 12, 2022 01:09
@daniel-white daniel-white merged commit ea5b445 into stoplightio:master Mar 17, 2022
This was referenced Jul 2, 2024
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.

None yet

5 participants