fix(twilio-run): migrate from ngrok to @ngrok/ngrok for Apple Silicon support#546
Conversation
… support Fixes spawn error -88 on Apple Silicon (M1/M2) Macs by migrating from the ngrok package to the official @ngrok/ngrok package. Key changes: - Replace ngrok@^4.3.3 with @ngrok/ngrok@^1.4.1 in optionalDependencies - Pure JavaScript implementation eliminates architecture-specific binary issues - Automatically reads authtoken from ngrok config files (~/.ngrok2/ngrok.yml or ~/Library/Application Support/ngrok/ngrok.yml) - Backward compatible: --ngrok=myapp converts to myapp.ngrok.io - Supports full domain format: --ngrok=myapp.ngrok.io - Enhanced error messages with troubleshooting guidance - Added proper cleanup handlers for graceful tunnel shutdown
🦋 Changeset detectedLatest commit: 11cc112 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR migrates from the deprecated ngrok package to the official @ngrok/ngrok package to resolve compatibility issues on Apple Silicon (M1/M2/M3) Macs. The migration updates the ngrok integration to use the new API while maintaining backward compatibility with existing command-line flag formats.
Changes:
- Replaced
ngrokpackage dependency with@ngrok/ngrokv1.4.1 - Updated ngrok tunnel creation to use the new
forward()API instead ofconnect() - Added automatic authtoken reading from ngrok configuration files
- Enhanced error handling with troubleshooting guidance
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/twilio-run/package.json | Updated optional dependency from ngrok to @ngrok/ngrok v1.4.1 |
| packages/twilio-run/src/flags.ts | Updated ngrok flag description to clarify domain format options |
| packages/twilio-run/src/config/start.ts | Implemented new ngrok integration with forward() API, authtoken reading, signal handlers, and enhanced error messages |
| packages/twilio-run/tests/config/start.test.ts | Updated test mocks to reflect new forward() API and added tests for error handling and domain conversion |
| packages/twilio-run/tests/config/start.withoutNgrok.test.ts | Updated mock to reference new @ngrok/ngrok package name |
| packages/twilio-run/README.md | Updated documentation examples to show both subdomain and full domain format options |
| packages/twilio-run/CHANGELOG.md | Added changelog entry documenting the migration and its benefits |
| .changeset/migrate-ngrok-package.md | Added changeset for automated version management |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address all 8 Copilot review comments on ngrok migration: 1. Error message accuracy - Replace misleading npm install suggestion with helpful guidance about binary execution issues 2. Test coverage - Add comprehensive test suite for getNgrokAuthToken() with 7 test cases covering all scenarios 3. Documentation consistency - Use "Apple Silicon" instead of chip versions 4. Type safety - Import and use official Listener type from @ngrok/ngrok 5. Import consistency - Move os and fs imports to top-level ES6 imports 6. Signal handler placement - Move to registerNgrokCleanup() function with idempotency check to avoid test environment issues 7. Listener tracking - Close existing tunnels before creating new ones to prevent orphaned tunnels 8. Error code clarity - Add comments explaining errno -88 is macOS-specific Additional: - Document pre-existing TypeScript compilation errors in TEST_FAIL.md - Export getNgrokAuthToken() for testing - Add robust error handling with type checks All changes maintain backward compatibility and follow codebase patterns. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tegration
- Update @ngrok/ngrok from v1.4.1 to v1.7.0 (latest stable)
- Fix domain detection: use .includes('.ngrok.') to handle all ngrok TLDs
- Fix authtoken regex to ignore inline comments in config files
- Add Windows config path support for authtoken auto-detection
- Add comprehensive test coverage for authtoken integration
- Add test for domain detection edge cases (e.g., 'my.app')
- Add test for authtoken parsing with inline comments
- Update changeset to clarify version migration and improvements
Addresses all Copilot recommendations in PR review.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
co-pilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
co-pilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
co-pilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Reverts the ngrok flag description to match the existing snapshot test. The new description caused snapshot test failures across all CI platforms. Also fixes 3 tests that were failing due to reading the real ngrok config from the system (~/.ngrok2/ngrok.yml). These tests now mock fs.existsSync to prevent reading real config files, ensuring consistent test behavior. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Overall looks great, a couple of small questions to double-check the AI reviews and remove the test failures file. |
- Use process.once() instead of process.on() for signal handlers to prevent memory leaks - Remove ngrokCleanupRegistered flag (no longer needed with process.once()) - Update changeset to use accurate "cross-platform binary management" terminology - Add test coverage for Windows AppData ngrok config path Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Update regex from /authtoken:\s*(\S+)/ to /authtoken:\s*([^\s#]+)/ to explicitly exclude '#' characters. This prevents incorrect token capture if inline comments lack spacing, making the parser more defensive. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses GitHub Copilot review #3679768013 with comprehensive fixes.
1. Optional Dependency Type (CRITICAL):
- Remove 'import type { Listener } from @ngrok/ngrok' (start.ts:17)
- Define local NgrokListener interface to avoid compile-time dependency
- Update type references to use local interface (start.ts:64, 87)
- Eliminates compile-time dependency on optional @ngrok/ngrok package
- Consumers can now compile without installing optional dependency
2. Nested Spy Conflicts (8 instances fixed):
- Add readFileSpy to beforeEach/afterEach for shared spy management
- Remove local spy creation in 8 tests that shadowed beforeEach spies
- Use shared existsSpy and readFileSpy with mockReturnValueOnce()
- Eliminates double-spying and variable shadowing issues
- Tests: converts subdomain, preserves full domain, converts non-ngrok,
preserves ngrok.dev, preserves ngrok-free.app, appends .ngrok.io,
passes authtoken, does not read real config
3. Invalid Jest Assertion (1 instance):
- Fix invalid 'resolves.not.toThrow()' pattern (start.test.ts:790)
- Replace with simple 'await sigintHandler()' (fails if rejected)
- Uses correct promise assertion mechanics
Verification:
- All 48 tests passing
- Zero flakiness across 3 test runs
- TypeScript compiles successfully without optional dependencies
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses GitHub Copilot review #3680088233: 1. Add explicit type annotations for ngrok and listener variables (start.ts:203, 225) - Fixes implicit-any violations under strict TypeScript mode - Improves type safety and IDE support 2. Fix tunnel leak risk (start.ts:258-268) - Close listener (best-effort) before throwing validation error - Prevents orphaned ngrok tunnels when url() returns null 3. Fix signal handler async/sync mismatch (start.ts:67-78) - Change signal handlers from async to sync (Node.js requirement) - Make cleanup fire-and-forget since process.exit() is immediate - Caught by new ESLint configuration Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses all 4 comments from GitHub Copilot review #3680170384:
1. Add { virtual: true } to jest.mock() for @ngrok/ngrok
- start.test.ts: Add virtual flag to mock optional dependency
- start.withoutNgrok.test.ts: Add virtual flag to mock optional dependency
- Prevents "Cannot find module" errors when optional dep not installed
2. Add { virtual: true } to second test file (same issue as twilio-labs#1)
- Ensures consistent mocking pattern across all test files
3. Remove manual "Unreleased" section from CHANGELOG.md
- Project uses Changesets for changelog management
- Manual entries conflict with automated release tooling
- Release notes are in .changeset/migrate-ngrok-package.md
4. Fix error message to reference correct package name
- Changed "ngrok" to "@ngrok/ngrok" in require() error message
- Improves clarity (old package was "ngrok", new package is "@ngrok/ngrok")
Changes:
- packages/twilio-run/src/config/start.ts (error message)
- packages/twilio-run/__tests__/config/start.test.ts (virtual mock)
- packages/twilio-run/__tests__/config/start.withoutNgrok.test.ts (virtual mock + test expectations)
- packages/twilio-run/CHANGELOG.md (remove manual section)
Test Results: All 50 tests passing (48 + 2)
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@jannoteelem I have completed all the co-pilot changes. I had to install local ESLint to catch most of them. I have not checked it in on the project, but checking if I should do this? What is the convention here? |
|
Congratulations on your first contribution to the Serverless Toolkit! |
|
@deshartman please create a separate PR for ESLint. It sounds like a useful addition. |
Adds comprehensive ESLint setup to proactively catch common code quality issues flagged by GitHub Copilot during PR reviews, including: - Implicit any types and unsafe type operations - Floating promises and misused promises - Unused variables - Jest best practices The configuration is tuned for this TypeScript monorepo and includes lenient rules for test files where stricter typing is less critical. This change helps prevent issues like those caught in twilio-labs#546 where Copilot identified type safety and promise handling concerns. Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Migrates from the community
ngrokpackage to the official@ngrok/ngrokpackage to resolve spawn error -88 on Apple Silicon (M1/M2/M3) Macs and improve overall reliability.Changes
Migration
ngrok@^4.3.3with@ngrok/ngrok@^1.7.0(official ngrok package)Bug Fixes & Improvements
.includes('.')to.includes('.ngrok.')to correctly handle subdomains like "my.app"/authtoken:\s*(.+)/to/authtoken:\s*(\S+)/to ignore inline comments in config files%USERPROFILE%\AppData\Local\ngrok\ngrok.ymlon WindowsBackward Compatibility
--ngrok=myappformat (automatically converts tomyapp.ngrok.io)--ngrok=myapp.ngrok.io.ngrok.io,.ngrok.dev, etc.Auto-detection
~/.ngrok2/ngrok.yml(legacy, cross-platform)~/Library/Application Support/ngrok/ngrok.yml(macOS)%USERPROFILE%\AppData\Local\ngrok\ngrok.yml(Windows)Benefits
Testing
ngrok.forward()--ngrokflag formatsRelated Issues
Fixes issues with ngrok failing on Apple Silicon Macs with spawn error -88.
🤖 Generated with Claude Code