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 turbo usage in tests #44715

Conversation

jankaifer
Copy link
Contributor

@jankaifer jankaifer commented Jan 9, 2023

  • install ts-node
  • start packing things with turbo properly
  • refactored repo-setup.js to use new turbo packing
  • migrated to .mts for scripts

I made the script pnpm pack compatible for easier switching in the future.
Right now pnpm pack can't be used because it doesn't include swc binaries in the resulting tarball. Will solve it in a separate PR.

The stats action is failing because canary doesn't have the test-pack action in turbo.json.

So we can either merge this in 2 phases or just ignore that one failing Action.

Testing

I tried testing this by editing the next core, but could someone double check please?

Questions

I made test-pack turbo task depend only on source files (that are not gitignored). Which is not great, it would be better if I could mark it as dependent on build but that could be a bit slower since people use dev.

Also If the dev produces something janky, test-pack might cache it.
I'm not sure how common that is.

@ijjk ijjk added create-next-app Related to our CLI tool for quickly starting a new Next.js application. created-by: Next.js team PRs by the Next.js team. type: next labels Jan 9, 2023
@jankaifer jankaifer marked this pull request as draft January 9, 2023 09:23
@jankaifer
Copy link
Contributor Author

The stats action is failing because canary doesn't have the test-pack action in turbo.json.

So we can either merge this in 2 phases or just ignore that one failing Action.

@jankaifer
Copy link
Contributor Author

It's weird I ran those CI actions locally (that docker command on top) and it passes 🤷.

@jankaifer
Copy link
Contributor Author

Hmm, it passes now.
Build, test, and deploy / Test Development (18, 3) (pull_request) seems to be actual regression. I'll need to check the updated packages and what caused the issue (i installed ts-node).

@jankaifer jankaifer marked this pull request as ready for review January 12, 2023 10:06
timneutkens
timneutkens previously approved these changes Jan 17, 2023
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Looks like the pnpm pack bug was fixed.

pnpm/pnpm#2941 (comment)

Should we try updating pnpm to the latest version?

@@ -8,6 +8,9 @@
"url": "vercel/next.js",
"directory": "packages/eslint-config-next"
},
"scripts": {
"test-pack": "cd ../../ && pnpm test-pack eslint-config-next"
Copy link
Member

Choose a reason for hiding this comment

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

This seems redundant that the package has to pass the name. Can we update the script to derive the current package by reading process.cwd() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I dig into it. And I would need to repeat that ts-node magical initialization.
Not worth it. But good point.

@jankaifer
Copy link
Contributor Author

Should we try updating pnpm to the latest version?

I want to merge this one. And we probably want to merge pnpm upgrade as its own PR.

@jankaifer jankaifer merged commit d7307cf into vercel:canary Jan 18, 2023
@jankaifer jankaifer deleted the jankaifer/next-344-try-rethinking-turbo-usage-in-tests-and branch January 18, 2023 19:35
@ijjk ijjk mentioned this pull request Jan 19, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
create-next-app Related to our CLI tool for quickly starting a new Next.js application. created-by: Next.js team PRs by the Next.js team. type: next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants