Skip to content

test: bump version of pinned @astrojs/language-server #261

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 19, 2024
Merged

test: bump version of pinned @astrojs/language-server #261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6cf9d91
ci: use PAT for create-tutorial releases
AriPerkkio Aug 19, 2024
a1f7323
test: dont pin `@astrojs/language-server`
AriPerkkio Aug 19, 2024
152deef
fix: code review
AriPerkkio Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/publish-create-tutorial.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ jobs:
body: 'Bump `create-tutorial` to version ${{ inputs.version }}.'
reviewers: SamVerschueren,d3lm,Nemikolh,AriPerkkio
branch: chore/release-create-tutorial-${{ inputs.version }}
token: ${{ secrets.GITOPS_REPO_PAT }}

publish_release_create_tutorial:
name: Publish Release create-tutorial
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/tests/create-tutorial.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ async function runPnpmInstall(dest: string, baseDir: string) {

packageJson.pnpm = {
overrides: {
'@astrojs/language-server': '2.11.1',
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be safer to pin it to the latest?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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 😭

Copy link
Member Author

Choose a reason for hiding this comment

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

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. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, sounds good!

'@astrojs/language-server': '2.14.1',
'@tutorialkit/astro': `file:${baseDir}/packages/astro`,
'@tutorialkit/components-react': `file:${baseDir}/packages/components/react`,
'@tutorialkit/runtime': `file:${baseDir}/packages/runtime`,
Expand Down