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

Yarn 4 upgrade fixes #21818

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

Conversation

crutchcorn
Copy link

This PR gets Yarn 4 (Berry) into a working state, based off of the work done by @ronaldlangeveld in #21292

It does this by:

  • Adding missing packages
  • Resolving things manually as-needed
  • Moved things from the Ghost ESLint plugin to the root deps temporarily while I wait for some feedback from @JoshuaKGoldberg
  • Resolving merge conflicts
  • Upgrading Yarn to fix issues with lockfile generation
  • Clearing the lockfile to fix issues with lockfile

ronaldlangeveld and others added 14 commits October 14, 2024 04:54
no issue

- Yarn v4 introduces some breaking changes to the way flags work, so we
  had to change the way we install them on CI.
I've reached out to Josh Goldberg (Commiter of ESLint + Lead Maintainer of TypeScript-ESlint) to see if we can figure out the issues with the upstream eslint-plugin-ghost code
# Conflicts:
#	.gitignore
#	ghost/bookshelf-repository/package.json
#	nx.json
#	package.json
#	yarn.lock
@crutchcorn crutchcorn changed the base branch from yarn-upgrade to main December 6, 2024 01:39
@crutchcorn
Copy link
Author

crutchcorn commented Dec 6, 2024

OK, test and build both seem to pass for all folders now locally

@@ -116,17 +122,27 @@
},
"devDependencies": {
"@actions/core": "1.11.1",
"@kapouer/eslint-plugin-no-return-in-loop": "1.0.0",
"@typescript-eslint/eslint-plugin": "6.9.1",
"@typescript-eslint/parser": "6.9.1",

Choose a reason for hiding this comment

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

eslint-plugin-ghost has specific versions of tseslint packages, and is actually on the latest 8.17.0:

https://github.com/TryGhost/eslint-plugin-ghost/blob/0bc3217ebe1409c12496860bbef37c6d4bb3eabf/package.json#L36-L37

Having a different version here will give you two versions. eslint/eslint#15681 might be the issue?

I'm not super confident this is what you're seeing and haven't set all this stuff up locally to try. If this isn't the right fix, let me know and I can poke deeper.

Copy link
Author

Choose a reason for hiding this comment

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

Ahh, I see the confusion this is my bad.

A few things to note:

  1. The TryGhost/Ghost repo is using an older version of eslint-plugin-ghost that includes these specific versions
  2. I'm hoping to remove the need for the "install in the monorepo" peerDeps-style dep sharing.

Instead, I'd love to npm i eslint-plugin-ghost and have it handle the versions of @typescript-eslint and such for us

Choose a reason for hiding this comment

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

Alternative, are there blockers to just using flat config? 😁

Choose a reason for hiding this comment

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

Anyway, what folks normally do is require.resolve("@typescript-eslint/eslint-plugin) inside the plugin. That way it resolves to the specific string path within your node_modules/eslint-plugin/ghost/node_modules/ (if that's not deduplicated to something higher up). I'll try that out now...

Choose a reason for hiding this comment

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

I, ah, don't know why this isn't working: https://github.com/JoshuaKGoldberg/eslint-plugin-ghost/tree/require-resolve.

Steps to repro what I thought would fix things:

  1. Check out that branch locally
  2. Link the Ghost repo to that branch with "eslint-plugin-ghost": "file:../eslint-plugin-ghost" in the package.json's dependencies
  3. Remove all parser: '@typescript-eslint/parser' lines from ESLint config files in the Ghost repo

I do see console logs from the local package. But:

 Oops! Something went wrong! :(
      
      ESLint: 8.44.0
      
      ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin".
      
      (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/Users/josh/repos/Ghost/ghost/nql-filter-expansions".)
      
      It's likely that the plugin isn't installed correctly. Try reinstalling by running the following:
      
          npm install @typescript-eslint/eslint-plugin@latest --save-dev
      
      The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in ".eslintrc.js » plugin:ghost/ts".
      
      If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.

21/95 targets fail in yarn lint. 🤔

@crutchcorn crutchcorn mentioned this pull request Dec 6, 2024
1 task
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.

3 participants