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

CI failing with op_mob browserslist error #1242

Closed
cmorten opened this issue Jun 25, 2023 · 13 comments · Fixed by #1243 or #1262
Closed

CI failing with op_mob browserslist error #1242

cmorten opened this issue Jun 25, 2023 · 13 comments · Fixed by #1243 or #1262
Assignees

Comments

@cmorten
Copy link

cmorten commented Jun 25, 2023

  • @testing-library/dom version: latest
  • Testing Framework and version: N/A
  • DOM Environment: N/A

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 #1170 (comment).

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
Copy link
Member

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
Copy link
Member

eps1lon commented Jun 26, 2023

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

@MatanBobi
Copy link
Member

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

@MatanBobi MatanBobi reopened this Jun 28, 2023
@MatanBobi
Copy link
Member

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

@nickserv
Copy link
Member

So #1255 should fix this, right?

@MatanBobi
Copy link
Member

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
Copy link
Member

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

@MatanBobi
Copy link
Member

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
Copy link
Member

nickserv commented Sep 12, 2023

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

@eps1lon
Copy link
Member

eps1lon commented Sep 16, 2023

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.

@eps1lon eps1lon closed this as completed Sep 16, 2023
@eps1lon
Copy link
Member

eps1lon commented Sep 16, 2023

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
Copy link
Member

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 #1257 to use defaults and update the overrides dependencies. That will be good once #1262 will be merged.

@eps1lon
Copy link
Member

eps1lon commented Sep 17, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment