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 dependencies to avoid build failure #631

Merged
merged 8 commits into from
Sep 25, 2024
Merged

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Sep 24, 2024

Prerequisites checklist

What is the purpose of this pull request?

What changes did you make? (Give an overview)

Upgraded the following packages to allow npm install to work without error:

  • typescript
  • rollup-plugin-dts
  • rollup

Related Issues

refs #630

Is there anything you'd like reviewers to focus on?

@brettz9 can you help me fix the failing test? I can't for the life of me figure out what it's trying to do, but it appears to not be handling TSArrayTypes properly.

@nzakas nzakas mentioned this pull request Sep 24, 2024
1 task
@snitin315
Copy link
Contributor

Upgraded the following packages to allow npm install to work without error:

I see only ESLint updated in package.json.

@mdjermanovic mdjermanovic changed the title fix: Update dependencies to avoid buildfailure fix: Update dependencies to avoid build failure Sep 24, 2024
@nzakas
Copy link
Member Author

nzakas commented Sep 24, 2024

Oops, looks like I missed a commit.

@nzakas
Copy link
Member Author

nzakas commented Sep 24, 2024

Now this is super weird, everything locally is working but the CI isn't. ☹️

@brettz9
Copy link
Contributor

brettz9 commented Sep 24, 2024

Yeah, I'm getting install errors before being able to look at tests. Did you do a fresh install at root?

@brettz9
Copy link
Contributor

brettz9 commented Sep 24, 2024

When I amend the tsc script to add tsc -v, it gives "Version 4.5.5", despite package.json having the ^5.6.2 version. That's where the warning is apparently coming from. I guess you don't want build:types as part of the prepare routine for the sake of first-time installs.

@nzakas
Copy link
Member Author

nzakas commented Sep 24, 2024

Yeah, I've tried removing prepare altogether but I'm still getting the same error. I'll push that commit.

@brettz9
Copy link
Contributor

brettz9 commented Sep 24, 2024

Even weirder is that when I remove that from the install script, the install proceeds and completes, saying that typescript 5.6.2 is installed but typescript is not present in node_modules.

@nzakas
Copy link
Member Author

nzakas commented Sep 24, 2024

I'm also seeing that the reported tsc version locally is 4.5.5, yet when I go into node_modules and look at the typescript package, it clearly says 5.6.2. 🤯

I can find any evidence of any package installing 4.5.5, though.

@brettz9
Copy link
Contributor

brettz9 commented Sep 24, 2024

The different set of packages in node_modules is changed to include the proper typescript version when I remove:

"workspaces": [
    "packages/*"
  ],

...though I know that's the whole point.

@nzakas
Copy link
Member Author

nzakas commented Sep 24, 2024

Okay, I think I know what's happening. The packages are installed in alphabetical order, so something in eslint-scope is causing the old TypeScript to be installed. Commit incoming.

@nzakas
Copy link
Member Author

nzakas commented Sep 24, 2024

Well, different error, but old tsc is still being installed. 🤔

@brettz9
Copy link
Contributor

brettz9 commented Sep 24, 2024

Could this be of significance?

After running npm i -ws, I get:

npm warn workspaces eslint-scope in filter set, but no workspace folder present
npm warn workspaces eslint-visitor-keys in filter set, but no workspace folder present
npm warn workspaces espree in filter set, but no workspace folder present

Likewise do I get the error when installing within the directory.

@brettz9
Copy link
Contributor

brettz9 commented Sep 24, 2024

tsd is installing the bad TS version (one reason I like having package lock files built to see this kind of info easily)

@brettz9
Copy link
Contributor

brettz9 commented Sep 24, 2024

Oddly, when upgrading tsd, I get an even older version of typescript showing up within package.json tsc -v; it is apparently using my global TS version now.

@brettz9
Copy link
Contributor

brettz9 commented Sep 24, 2024

The TS version is fixed when I change this in the root package.json:

  "workspaces": [
    "packages/eslint-scope",
    "packages/eslint-visitor-keys",
    "packages/eslint-espree"
  ],

@nzakas
Copy link
Member Author

nzakas commented Sep 24, 2024

The top-level package.json is working as-is on CI. Just need to tweak the test script to include build since I removed prepare.

So I think now I can ask you about the failing test in eslint-visitor-keys. 😄

@brettz9
Copy link
Contributor

brettz9 commented Sep 24, 2024

Ok, I can look into it, but it may be working on CI merely because it has a higher global TS version installed. With the change I made, the warnings like this are now gone also:

npm warn workspaces eslint-visitor-keys in filter set, but no workspace folder present

@brettz9
Copy link
Contributor

brettz9 commented Sep 24, 2024

I'm afraid this was a bit fragile to begin with (at least incomplete), and now with updates, the script can't accommodate. I think you'll either need to build the keys again manually or find someone with greater bandwidth than myself to adapt the script. I'm finding it difficult to revisit the code unfortunately.

@nzakas
Copy link
Member Author

nzakas commented Sep 25, 2024

@brettz9 okay, thanks.

@nzakas
Copy link
Member Author

nzakas commented Sep 25, 2024

I've gone through and remove the tools because they don't work anymore. I think we're okay to update the keys manually when needed.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit e8cd107 into main Sep 25, 2024
11 checks passed
@mdjermanovic mdjermanovic deleted the visitor-keys-fix branch September 25, 2024 16:46
@github-actions github-actions bot mentioned this pull request Sep 25, 2024
@mdjermanovic mdjermanovic mentioned this pull request Sep 25, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

4 participants