Skip to content

Conversation

@ckeshava
Copy link
Contributor

High Level Overview of Change

This is an attempt to supercede this PR: #1202. cheerio is a dependency of enzyme library. The explorer code base does not have usages of the cheerio library.

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Translation Updates
  • Release

Codebase Modernization

  • Updated files to React Hooks
  • Updated files to TypeScript
  • Updated tests to React Testing Library

Before / After

Test Plan

@mvadari
Copy link
Collaborator

mvadari commented Jul 30, 2025

I disagree with this PR. The reason it's in the package.json is because the higher versions break tests. And the only way to pin a version is to specify it like that - otherwise npm will automatically try to update it. Tests only pass because npm uses npm ci to install packages, and you haven't changed the version in package-lock.json.

@ckeshava
Copy link
Contributor Author

Tests only pass because npm uses npm ci to install packages, and you haven't changed the version in package-lock.json

I used npm i --legacy-peer-deps on my local machine. I don't see any errors. The legacy-peer-deps switch is required to resolve other dependencies ([email protected], [email protected]).

And the only way to pin a version is to specify it like that - otherwise npm will automatically try to update it.

If enzyme wants to use versions of cheerio only up until 1.0.0-rc.3, it is enzyme's responsibility to specify the correct semantic-version in its package.json file. Why should we bear the responsibility of pinning the correct version of a secondary dependency?

@mvadari
Copy link
Collaborator

mvadari commented Jul 30, 2025

I used npm i --legacy-peer-deps on my local machine. I don't see any errors. The legacy-peer-deps switch is required to resolve other dependencies ([email protected], [email protected]).

If you try to change/update any package, it'll usually trigger an attempted update in cheerio.

If enzyme wants to use versions of cheerio only up until 1.0.0-rc.3, it is enzyme's responsibility to specify the correct semantic-version in its package.json file. Why should we bear the responsibility of pinning the correct version of a secondary dependency?

It's our responsibility because our tests fail otherwise on the latest version of cheerio (see the dependabot PR). It's not enzyme's responsibility because they don't officially support React 17 anyways - we're using a 3rd party adapter to get it to work. This was the solution I could find to get tests working. If you can find a better solution, I'm all for it. I have an old PR (#1014) that's like 70% done to migrate off of enzyme, if you'd rather go that route.

For now, this change is unnecessary and will break things sooner rather than later, so why bother?

@ckeshava
Copy link
Contributor Author

ckeshava commented Jul 31, 2025

alright, I didn't realize that we were using an old version of React. I believe the latest version is at [email protected].

I do not have time to pick up on your PR at the moment, however I'll keep it in my mind 👍

@ckeshava ckeshava closed this Jul 31, 2025
@mvadari
Copy link
Collaborator

mvadari commented Jul 31, 2025

alright, I didn't realize that we were using an old version of React. I believe the latest version is at [email protected].

We can't upgrade due to our use of enzyme for tests - migrating off of it to RTL would allow us to upgrade.

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.

2 participants