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

feat: update browserslist and remove overrides workaround #1257

Closed

Conversation

MatanBobi
Copy link
Member

@MatanBobi MatanBobi commented Sep 10, 2023

What: Resolves #1242. This updates the browserslist entry in our package.json.

Why: As part of our major version, we're updating the browserslist support.

How: Ran npx browserslist defaults and updated the browserslist support in our package.json file. I've also removed the temporary workarounds introduced in #1243.

Checklist:

  • Documentation added to the
    docs site
  • Tests
  • TypeScript definitions updated
  • Ready to be merged

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 10, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f606268:

Sandbox Source
react-testing-library-examples Configuration
react-testing-library demo Issue #1242

@codecov
Copy link

codecov bot commented Sep 10, 2023

Codecov Report

Merging #1257 (f606268) into alpha (452097b) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             alpha     #1257   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines         1038      1038           
  Branches       349       349           
=========================================
  Hits          1038      1038           
Flag Coverage Δ
node-18 100.00% <ø> (ø)
node-20 100.00% <ø> (ø)

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

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@MatanBobi MatanBobi linked an issue Sep 10, 2023 that may be closed by this pull request
@MatanBobi MatanBobi linked an issue Sep 10, 2023 that may be closed by this pull request
nickserv

This comment was marked as outdated.

@MatanBobi
Copy link
Member Author

Alternatively can we set the browserslist to defaults (or something including Node) and pin it to prevent breakage?

Doing that will require us to use overrides since browserslist is not our direct dependency, it's from kcd-scripts. This means we'll need to maintain that overrides, that's why I prefer having it hard-coded here.
Wdyt?

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

A green build 🥳
I also prefer this, as its more flexible imho.

@nickserv
Copy link
Member

nickserv commented Sep 12, 2023

I don't think we should remove the overrides: The bug fix in this PR is unnecessary because it only targets npm versions installed by Node 16, which are no longer supported in the alpha branch as of #1255.

package.json Show resolved Hide resolved
Comment on lines -73 to -74
"browserslist": "4.21.8",
"caniuse-lite": "1.0.30001502",
Copy link
Member

@nickserv nickserv Sep 12, 2023

Choose a reason for hiding this comment

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

Can be reverted #1257 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

This was just a "hack" to make node 14 build pass but it didn't work so I think either way we won't need it.

"overrides": {
"browserslist": "4.21.8",
"caniuse-lite": "1.0.30001502"
},
Copy link
Member

@nickserv nickserv Sep 12, 2023

Choose a reason for hiding this comment

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

Can be reverted #1257 (comment)

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Let's do this as the last step right before shipping the next major. The browserslist override needs to stay. Otherwise it'll break again.

@nickserv
Copy link
Member

Can we merge this once we revert the overrides? Since we're already released alphas and this is also targeting the alpha branch, I'd prefer our CI to be passing sooner rather than later.

Copy link
Member

@nickserv nickserv left a comment

Choose a reason for hiding this comment

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

See my initial review (excluding the resolved comment) or @eps1lon's review

@eps1lon
Copy link
Member

eps1lon commented Oct 1, 2023

CI should be green with #1262.

We'll continue to need the overrides otherwise we'll encounter this issue again.

@eps1lon eps1lon deleted the branch testing-library:alpha April 8, 2024 11:59
@eps1lon eps1lon closed this Apr 8, 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.

CI failing with op_mob browserslist error
5 participants