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

Fix update core dependencies #1708

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MattiasBuelens
Copy link
Contributor

The "update core dependencies" workflow is broken again, see log:

Run npm i
npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR! 
npm ERR! While resolving: [email protected]
npm ERR! Found: [email protected]
npm ERR! node_modules/eslint
npm ERR!   dev eslint@"^9.1.1" from the root project
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peer eslint@"^8.56.0" from @typescript-eslint/[email protected]
npm ERR! node_modules/@typescript-eslint/parser
npm ERR!   dev @typescript-eslint/parser@"^7.7.1" from the root project
npm ERR!   peer @typescript-eslint/parser@"^7.0.0" from @typescript-eslint/[email protected]
npm ERR!   node_modules/@typescript-eslint/eslint-plugin
npm ERR!     dev @typescript-eslint/eslint-plugin@"^7.7.1" from the root project
npm ERR! 
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.

The problem is that ESLint version 9 has been released, but TypeScript ESLint doesn't support it yet. I worked around it by making ncu -u ignore eslint for now.

That said, perhaps it would be better if this workflow only updated the actual "core dependencies"? npm-check-updates accepts a filter argument, so we could do something like this instead:

ncu -u '/^@mdn/' '/^@webref/'

@saschanaz
Copy link
Contributor

saschanaz commented Apr 28, 2024

This has never been problem for this long, it has been okay for many years. I guess ESLint 9 contains a lot of huge breaking changes? And yes I think it would be better to have a separate job rather than having a temporary workaround.

(Edit: I mean, it's not "broken again", it has been broken with the same error for several weeks and I was expecting it would be solved very soon.)

@saschanaz
Copy link
Contributor

We can also try 8.0.0-alpha.3 🤔

@MattiasBuelens
Copy link
Contributor Author

ESLint 9 changed the default configuration format to the new "flat config". So we'd need to migrate to the new format, or throw ESLINT_USE_FLAT_CONFIG=false into the mix. But either way, it won't be an "automatic" upgrade, we need to make some changes.

Indeed, it looks like we'll need an alpha release of typescript-eslint for now, but I can give it a shot if you want?

@MattiasBuelens
Copy link
Contributor Author

Never mind, that alpha version still only supports ESLint v8. Support for ESLint v9 is still being worked on in typescript-eslint/typescript-eslint#9002, so we cannot update to v9 at the moment.

I'll see if I can migrate us to the flat config format already. That should hopefully make it easier to update to v9 once TypeScript ESLint adds support.

@MattiasBuelens
Copy link
Contributor Author

That should do it.

The next workflow run should properly update the dependencies again:

$ ncu -u -x eslint -x @eslint/js
Upgrading TypeScript-DOM-lib-generator/package.json
[====================] 24/24 100%

 @mdn/browser-compat-data  ^5.5.21  →  ^5.5.23
 @webref/css               ^6.12.7  →  ^6.12.9
 @webref/idl               ^3.46.1  →  ^3.47.1

Run npm install to install new versions.

@saschanaz
Copy link
Contributor

But again, should we try only updating "core dependencies" rather than doing a temporary workaround? I just opened #1709 which will let Dependabot to do things better, after that this job won't need to update other deps. (It did mostly because Dependabot had no group update, but now it has.)

@sandersn
Copy link
Member

sandersn commented May 8, 2024

I ended up reading #1709 and #1708 out of order. Now that #1709 is merged, only the eslint config update is needed, right?

@saschanaz
Copy link
Contributor

typescript-eslint/typescript-eslint#9002 is still open, that should happen too.

@MattiasBuelens
Copy link
Contributor Author

Yeah, I think I'll wait for TypeScript ESLint to get updated first.

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

Successfully merging this pull request may close these issues.

None yet

3 participants