search

testing-library / dom-testing-library

🐙 Simple and complete DOM testing utilities that encourage good testing practices.
https://testing-library.com/dom
MIT License
3.28k stars 467 forks source link

CI failing with op_mob browserslist error #1242

Closed cmorten closed 1 year ago

cmorten commented 1 year ago

Relevant code or config:

N/A

What you did:

Run npm run setup -s locally or in CI.

What happened:

$ kcd-scripts build  --no-ts-defs --ignore "**/__tests__/**,**/__node_tests__/**,**/__mocks__/**" && kcd-scripts build --no-ts-defs --bundle --no-clean
Error [BrowserslistError]: Unknown version 64 of op_mob

Reproduction:

npm run setup -s

Problem description:

op_mob 64 is not supported, as noted in https://github.com/testing-library/dom-testing-library/pull/1170#issuecomment-1595300619.

Suggested solution:

Move to use op_mob 73 provided it doesn’t result in a breaking change, otherwise a different config to unblock build.

MatanBobi commented 1 year ago

Hi @cmorten :) I already pushed a fix to your PR for this one, I saw this before and didn't get a chance to fix it.

eps1lon commented 1 year ago

Please fix this in a different PR so that we can better bisect/revert later.

MatanBobi commented 1 year ago

I'm re-opening this because what we have at the moment is a workaround (we've pinned the version) :)

MatanBobi commented 1 year ago

This issue is more urgent than we thought because our workaround fails our node 14 CI because npm 6 doesn't support overrides.

nickserv commented 1 year ago

So #1255 should fix this, right?

MatanBobi commented 1 year ago

So #1255 should fix this, right?

It should fix the failure but it's still a workaround. We should probably update the browsers we support at some point I guess.

nickserv commented 1 year ago

That's just a matter of updating browserslist, right? Since we're doing a major anyway, let's include a fix then.

MatanBobi commented 1 year ago

That's just a matter of updating browserslist, right? Since we're doing a major anyway, let's include a fix then.

Yes, that's a really good point. I'll do it and create a PR to the alpha branch.

nickserv commented 1 year ago

This issue seems to only reproduce with Node 16, which we're dropping support for in DTL 10.

eps1lon commented 1 year ago

It should fix the failure but it's still a workaround. We should probably update the browsers we support at some point I guess.

It's the other way around. https://github.com/testing-library/dom-testing-library/pull/1243 did fix it permanently. Bumping op_mob is the workaround since it'll break again eventually. We'd have to constantly bump browserslist and ship majors to keep CI green. Locking it is the permanent fix that should stay.

That doesn't mean we can't update browserslist in the next major though.

eps1lon commented 1 year ago

Problem is that we use the bundled NPM version and the one bundled with Node.js 14 doesn't support overrides. Pinning NPM version which makes sense anyway. We don't want to test different NPM versions. Just Node.js versions.

MatanBobi commented 1 year ago

It should fix the failure but it's still a workaround. We should probably update the browsers we support at some point I guess.

It's the other way around. #1243 did fix it permanently. Bumping op_mob is the workaround since it'll break again eventually. We'd have to constantly bump browserslist and ship majors to keep CI green. Locking it is the permanent fix that should stay.

That doesn't mean we can't update browserslist in the next major though.

I understand what you're saying and that's definitely a valid point. If that's the case, shouldn't this be pinned in kcd-scripts which defines them? When using overrides it feels like we're solving the issue just for us. I can update https://github.com/testing-library/dom-testing-library/pull/1257 to use defaults and update the overrides dependencies. That will be good once #1262 will be merged.

eps1lon commented 1 year ago

If kcd-scripts can pin browserslist, I'm all for it.