-
Notifications
You must be signed in to change notification settings - Fork 752
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
WIP: Ngrok Debugger Implementation #2032
Conversation
rerendering debug console on props change Error viewer console completed
Copy file spec completed Ngrok tunnel action spec updated
Ngrok module tests Testing tunnel error generation and capture scenarios
Code cleanup Renaming functions, test names, using shorthands wherever applicable
Updated icon for ngrok Status
Lock files updated after pull from master
@srinaath will remove the clients package-lock.json |
packages/app/client/.gitignore
Outdated
@@ -2,6 +2,7 @@ | |||
|
|||
# dependencies | |||
/node_modules | |||
package-lock.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could instead add an entry to the root level .gitignore
that ignores every lock file except for the root one?
/packages/**/package-lock.json
and then delete the sub-root-level lock files?
logPath: string; | ||
postmanCollectionPath: string; | ||
tunnelStatus: TunnelStatus; | ||
lastTunnelStatusCheckTS: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider expanding to lastTunnelStatusCheckTimestamp
for readability
fs.copyFile(sourcePath, destinationPath, err => { | ||
if (err) { | ||
// eslint-disable-next-line no-console | ||
console.error(`Error copying file: ${err}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe move this to the reject
call so that it is surfaced to whatever context calls it:
reject(Error copying file: ${err});
// OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION | ||
// WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
|
||
export default { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we prefer to use explicitly named exports in this project:
export const PostmanNgrokCollection = { ... };
protected async copyFile(sourcePath: string, destinationPath: string) { | ||
try { | ||
await copyFileAsync(sourcePath, destinationPath); | ||
return true; | ||
} catch (ex) { | ||
return false; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this returns true and false? The only code that calls this in your PR calls it and doesn't worry about the return value because it is being called as the resolver
of the executeCommand()
action
packages/app/main/src/ngrok.ts
Outdated
@@ -93,7 +99,30 @@ export class NgrokInstance { | |||
return this.pendingConnection; | |||
} | |||
await this.getNgrokInspectUrl(options); | |||
return this.runTunnel(options); | |||
const tunnelInfo: { url; inspectUrl } = await this.runTunnel(options); | |||
this.intervalForHealthCheck = setInterval(() => this.checkTunnelStatus.bind(this)(tunnelInfo.url), 60000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small optimization could be to pull the code that binds the function out of the setInterval()
call so that .bind()
is not called every time the function fires per interval:
const boundCheckTunnelStatus = this.checkTunnelStatus.bind(this);
this.intervalForHealthCheck = setInterval(() => boundCheckTunnelStatus(tunnelInfo.url), 60000);
1. Added space after breaks 2. Seperating container and actual visual component 3. Removing nbsp 4. Adding package-locks to gitignore Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
1.Using named exports 2. Returning errors from rejects 3. Removed dev artifacts Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
1. Added space after breaks 2. Seperating container and actual visual component 3. Removing nbsp 4. Adding package-locks to gitignore Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Pre feedback addressed 1.Using named exports 2. Returning errors from rejects 3. Removed dev artifacts Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Made sure all pacjage locks are removed and spacing between cases Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Untracked Package Locks Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Untracked and deleted package locks Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
packages/app/client/src/ui/editor/ngrokDebugger/ngrokDebuggerContainer.spec.tsx
Show resolved
Hide resolved
packages/app/client/src/ui/editor/ngrokDebugger/ngrokDebuggerContainer.tsx
Show resolved
Hide resolved
packages/app/client/src/ui/editor/ngrokDebugger/ngrokErrorHandler.tsx
Outdated
Show resolved
Hide resolved
packages/app/client/src/ui/editor/ngrokDebugger/ngrokErrorHandler.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more minor comments.
Comments tag removed Typos fixed Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Removed unused functions Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
return ( | ||
<> | ||
<legend> | ||
Looks like you have hit your free tier limit on connections to your tunnel. These are few solutions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed this one last time: These are few solutions.
Maybe try something like:
(Consider | Try) the following solutions.
or
Below you will find several possible solutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small comment. Once that's fixed I think this is good to go!
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! 👏
Thanks @tonyanziano |
Ngrok Debug Console
Added console window to track tunnel errors
Pinned React version
More unit tests coverage in future PR's