test: bump version of pinned @astrojs/language-server
#261
Conversation
|
|
AriPerkkio
force-pushed
the
fix/template-astro-deps
branch
from
d955da2 to
a1f7323
Compare
AriPerkkio
changed the title
@astrojs/check@astrojs/language-server
AriPerkkio
changed the title
@astrojs/language-server@astrojs/language-server
| @@ -240,7 +240,6 @@ async function runPnpmInstall(dest: string, baseDir: string) { | |||
|
|
|||
| packageJson.pnpm = { | |||
| overrides: { | |||
| '@astrojs/language-server': '2.11.1', | |||
There was a problem hiding this comment.
Wouldn't it be safer to pin it to the latest?
There was a problem hiding this comment.
As the tests are passing just fine there is no need to pin it at all. Let's use the version that transitive dependencies end up resolving. The overriding was added in #145.
There was a problem hiding this comment.
As the tests are passing just fine
They are now, but that's exactly the reason we pinned it in the first place. Because they were broken by an indirect update and I don't think it makes much sense to test in our CI the latest.
There was a problem hiding this comment.
Sounds like we should then apply this in the template too, so that end-users get the same experience that we are testing against.
Should we lock other transitive dependencies as well?
There was a problem hiding this comment.
Sounds like we should then apply this in the template too, so that end-users get the same experience that we are testing against.
I don't think we need to because we generate a lock file for a new tutorial so they already get a fixed experience and don't pull random updates from dependencies.
Should we lock other transitive dependencies as well?
We probably should.
The idea we originally had was to use lock files for those tests but edit them so that they use file dependencies for @tutorialkit/* packages. The problem was that pnpm format is just to complex to be a simple find and replace.
So with the hack in this file we basically partially did that.
I really don't like this test tbqh 😭
There was a problem hiding this comment.
I don't think we need to because we generate a lock file for a new tutorial so they already get a fixed experience
Those lockfiles don't contain this override. 🤔
There was a problem hiding this comment.
Hmm maybe I wasn't clear let me clarify.
The reason we have an override is to have a poor's man lockfile. Sam looked at adding one and doing a manual find and replace for @tutorialkit/* packages but it was too complex. Ideally this is what we would want.
We pinned the language server because it was the one causing an issue in that test with astro check. Astro check was picking up a newer version of the language server which surprisingly only failed when using file dependencies. When removing the file dependencies everything was working fine. So we pinned it.
New tutorials use a lock files that we generated when deploying a new version and we checked that everything was working. So new tutorial won't break by accidentally picking up a newer versions of a tutorialkit dependency.
Does that makes more sense?
AriPerkkio
changed the title
@astrojs/language-server@astrojs/language-server
create-tutorialv0.0.3 #260 (comment)