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

[Deps] Upgrade history package to 5.x #2531

Open
yurishkuro opened this issue Dec 26, 2024 · 9 comments · May be fixed by #2370
Open

[Deps] Upgrade history package to 5.x #2531

yurishkuro opened this issue Dec 26, 2024 · 9 comments · May be fixed by #2370
Labels
changelog:dependencies Update to dependencies good first issue help wanted

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Dec 26, 2024 •
edited
Loading

Some functionality has been deprecated in that library, we need to understand what the replacement is, and whether it is compatible with the modern React at all.
@yurishkuro yurishkuro linked a pull request Dec 26, 2024 that will close this issue
Open
4 tasks
@yurishkuro yurishkuro mentioned this issue Dec 26, 2024
Open
@jayesh9747
Copy link

@yurishkuro can you describe in details like what should excalty waht outcome you want ?

@yurishkuro
Copy link
Member Author

The desired outcome is that we don't have a stale dependency - we either upgrade to the latest version or if the dependency is deprecated altogether we find something else that is maintained.

@jayesh9747
Copy link

@yurishkuro, after updating the history version to 5.x, how can I check if it is compatible with this repository and passes all the tests?

@yurishkuro
Copy link
Member Author

You run the tests, npm run test, npm run lint.

We already know that it's not compatible, the automated upgrade is failing CI.

@jayesh9747
Copy link

@yurishkuro

As I searched on Google to find an alternative to History.js, I discovered that no direct alternatives are available. Therefore, I prefer to resolve the test cases that were filed instead.

Image

@yurishkuro
Copy link
Member Author

If it is unmaintained it's not enough to fix the tests, we need to actively get rid of it.

@yurishkuro
Copy link
Member Author

But fixing the test can be a first step.

@jayesh9747
Copy link

@yurishkuro sure , will PR very soon.

@manzil-infinity180
Copy link
Contributor

manzil-infinity180 commented Jan 8, 2025 •
edited
Loading

@yurishkuro

I upgraded to 5.X and tested it, two test cases failing one for the history.length and other one is for toMatchSnapshot()

 console.log
      {
        "index": 1,
        "action": "PUSH",
        "location": {
          "pathname": "/trace/MOCK-TRACE-ID",
          "search": "",
          "hash": "",
          "state": null,
          "key": "8kaa6msa"
        }
      }

      at Object.log (src/components/App/TraceIDSearchInput.test.js:50:13)

    console.log
      {
        "index": 0,
        "action": "POP",
        "location": {
          "pathname": "/",
          "search": "",
          "hash": "",
          "state": null,
          "key": "jxeeyogw"
        }
      }

      at Object.log (src/components/App/TraceIDSearchInput.test.js:57:13)

  ● <TraceIDSearchInput /> › does not push to history on falsy input value

    expect(received).toEqual(expected) // deep equality

    Expected: 1
    Received: undefined

      57 |     console.log(JSON.stringify(history, null, 2));
      58 |     expect(history.action).toEqual('POP');
    > 59 |     expect(history.length).toEqual(1);
         |                            ^
      60 |   });
      61 | });
      62 |

      at Object.toEqual (src/components/App/TraceIDSearchInput.test.js:59:28)

(node:17363) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
 FAIL  src/components/App/index.test.js
  ● JaegerUIApp › does not explode

    expect(received).toMatchSnapshot()

    Snapshot name: `JaegerUIApp does not explode 1`

    - Snapshot  - 4
    + Received  + 6

as we clearly see the history.length is undefined so test are failing

test reference link/code

jaeger-ui/packages/jaeger-ui/src/components/App/TraceIDSearchInput.test.js

Lines 43 to 58 in c28adbb

it('pushes input id to history', () => {
const traceId = 'MOCK-TRACE-ID';
const idInput = screen.getByPlaceholderText('Lookup by Trace ID...');
fireEvent.change(idInput, { target: { value: traceId } });
fireEvent.submit(screen.getByTestId('TraceIDSearchInput--form'));
expect(history.length).toEqual(2);
expect(history.location.pathname).toEqual(`/trace/${traceId}`);
});
it('does not push to history on falsy input value', () => {
fireEvent.submit(screen.getByTestId('TraceIDSearchInput--form'));
expect(history.length).toEqual(1);
});
});

jaeger-ui/packages/jaeger-ui/src/components/App/index.test.js

Lines 20 to 25 in c28adbb

describe('JaegerUIApp', () => {
it('does not explode', () => {
const wrapper = shallow(<JaegerUIApp />);
expect(wrapper).toMatchSnapshot();
});
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
changelog:dependencies Update to dependencies good first issue help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update history package and fix failing test cases DhairyaMajmudar/jaeger-ui
3 participants
@yurishkuro @manzil-infinity180 @jayesh9747