fix(ci): override @astrojs/language-server with an older version #145
Conversation
|
|
| @@ -18,7 +18,7 @@ interface PackageJson { | |||
| } | |||
|
|
|||
| const TUTORIALKIT_VERSION = pkg.version; | |||
| const REQUIRED_DEPENDENCIES = ['@tutorialkit/runtime', '@webcontainer/api', 'nanostores', '@nanostores/react']; | |||
| const REQUIRED_DEPENDENCIES = ['@tutorialkit/runtime', '@webcontainer/api', 'nanostores', '@nanostores/react', 'kleur']; | |||
There was a problem hiding this comment.
After moving the CLI test from npm to pnpm it revealed a bug in our eject command. 🎉
|
|
||
| packageJson.pnpm = { | ||
| overrides: { | ||
| '@astrojs/language-server': '2.11.1', |
There was a problem hiding this comment.
This is the version that we were using before that didn't exhibit that issue. I still think that doing the file:.. thing below is wrong. I'm wondering if moving to use .tar.gz files would solve the problem because then their node_modules would be entirely managed by the package manager.
| - prettier-plugin-astro | ||
| dev: true | ||
|
|
||
| /@astrojs/[email protected]: |
There was a problem hiding this comment.
We now also have a single version of @astrojs/compiler instead of 2
Deploying tutorialkit-demo-page with
|
| Latest commit: |
00aedc0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5461f817.tutorialkit-demo-page.pages.dev |
| Branch Preview URL: | https://joan-fix-ci-error.tutorialkit-demo-page.pages.dev |
Deploying tutorialkit-docs-page with
|
| Latest commit: |
e932a72
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cbd31327.tutorialkit-docs-page.pages.dev |
| Branch Preview URL: | https://joan-fix-ci-error.tutorialkit-docs-page.pages.dev |
| if (process.platform !== 'win32') { | ||
| await fs.rm(path.join(dest, 'node_modules'), { force: true, recursive: true, maxRetries: 5 }); | ||
| } | ||
|
|
||
| const projectFiles = await fs.readdir(dest, { recursive: true }); | ||
| let projectFiles = await fs.readdir(dest, { recursive: true }); | ||
|
|
||
| if (process.platform === 'win32') { | ||
| projectFiles = projectFiles.filter((filePath) => !filePath.startsWith('node_modules')); | ||
| } |
There was a problem hiding this comment.
I really don't like this hack but I couldn't find any other better way to remove this folder on Windows.
It seems that the issue is that when we try to delete the node_modules, we also delete esbuild.exe which get an EBUSY error. If we use the maxRetry property for that usecase, it times out.
My guess is that at this point there's still an esbuild process running. However, I don't know what spawned that esbuild process and I don't want to try to randomly kill all esbuild processes which would be way worse.
There was a problem hiding this comment.
It's kinda hacky but package managers and node's resolving is confusing some times. I looked at the error locally quickly and didn't find better solution. I was comparing pnpm-lock.yaml of the generated project and locally linked packages and didn't see any issues. 🤷
These changes also made the think that we should add post-release test cases that are pure e2e tests using packages from NPM.
|
@AriPerkkio Yeah I agree, I think that would be nice to do that. The more I think about it, the more I want to remove those tests using the |
This PR fixes the error encountered in CI after the latest update to
@astrojs/language-server. When we don't use file dependencies there are no issue. Runningastro checkon the template works completely fine and on a new project that use our npm deps it also works.My guess is that our use of
file://dependency in our test is kinda broken because some dependencies that should be shared aren't. I haven't been able exactly to put my finger on which one but having duplicate of the same dependency even if they are at the same version (here@astrojs/language-server) didn't work.Note that this PR also updates a few dependencies and dedupe some of them (like the
prettier-plugin-astroone).