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: reduce turbo errors on local dev, enable running e2e in built mode #259

Merged
merged 5 commits into from
Oct 24, 2024

Conversation

stefl
Copy link
Contributor

@stefl stefl commented Oct 24, 2024

Description

Incremental improvements to try to reduce turbo errors and test timeouts.

  • Removes Sentry wrapper for turbo mode
  • Adds some next.config settings for turbo mode
  • Adds a script to run e2e on a built server to reduce timeouts in tests

Turbo mode:

  • There seems to be a problem with Sentry whereby when you run the server in turbo mode, if you make changes, you'll see an error bounday issue about modules no longer being available in the factory
  • The guidance from Next is to not uses the withSentry wrapper in turbo mode, and to add some settings to next.config
  • This seems to have reduced the issue for me but not fixed it entirely

Running the e2e server in built mode could mean that:

  • We can run a separate server for e2e tests locally, and continue working on the dev server while the tests are running
  • Tests can run in parallel rather than one at a time

Copy link

vercel bot commented Oct 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
oak-ai-lesson-assistant ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 24, 2024 11:38am

Copy link

github-actions bot commented Oct 24, 2024

Playwright test results

passed  13 passed
skipped  1 skipped

Details

report  Open report ↗︎
stats  14 tests across 13 suites
duration  1 minute, 32 seconds
commit  dcde081

Skipped tests

No persona › tests/auth.test.ts › authenticate through Clerk UI

Copy link

"dev-turbo": "pnpm with-env node scripts/increase-listeners.js next dev --port 2525 --turbo | pino-pretty -C",
"dev-server": "pnpm with-env node scripts/increase-listeners.js next dev --port 2525 | pino-pretty -C",
"dev": "FORCE_COLOR=1 concurrently \"pnpm dev-server\" \"node scripts/local-dev.mjs\"",
"dev-turbo": "FORCE_COLOR=1 concurrently \"pnpm dev-server-turbo\" \"node scripts/local-dev.mjs\"",
Copy link
Collaborator

Choose a reason for hiding this comment

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

For my benefit, what tool needs FORCE_COLOR? Is it because concurrently is getting in the way? Could you add it to .env like DEBUG_COLORS?

I'm not convinced that we actually need the concurrent local dev server any more. I haven't noticed any issues having it disabled in the last week

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could probably remove it now, yes. Turbo seems to be so much better that I am no longer running it.

module.exports = withSentryConfig(module.exports, {
// For all available options, see:
// https://github.com/getsentry/sentry-webpack-plugin#options
if (!process.env.TURBOPACK) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see that there's a benefit for not having the monitoring instrumentation in here. But what if we need to test sentry locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, running the server in non-turbo dev mode would let you test it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The benefit is just to try to remove the sentry / turbo incompatibility

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that we should only have the turbo mode as default, to be honest

@stefl stefl merged commit afb4535 into main Oct 24, 2024
20 checks passed
@stefl stefl deleted the fix/local_dev branch October 24, 2024 11:49
@oak-machine-user
Copy link
Collaborator

🎉 This PR is included in version 1.12.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants