This repository was archived by the owner on Jan 5, 2026. It is now read-only.
Merged
Conversation
added 13 commits
January 5, 2020 20:50
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
requested review from
a-b-r-o-w-n,
compulim,
corinagum,
cwhitten,
tdurnford and
tonyanziano
as code owners
srinaath
changed the title
Member
|
@srinaath will remove the clients package-lock.json |
added 4 commits
January 6, 2020 16:06
tonyanziano
suggested changes
Jan 8, 2020
|
|
||
| # dependencies | ||
| /node_modules | ||
| package-lock.json |
Contributor
There was a problem hiding this comment.
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; |
Contributor
There was a problem hiding this comment.
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}`); |
Contributor
There was a problem hiding this comment.
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 { |
Contributor
There was a problem hiding this comment.
we prefer to use explicitly named exports in this project:
export const PostmanNgrokCollection = { ... };
Comment on lines
+200
to
+207
| protected async copyFile(sourcePath: string, destinationPath: string) { | ||
| try { | ||
| await copyFileAsync(sourcePath, destinationPath); | ||
| return true; | ||
| } catch (ex) { | ||
| return false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
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
| 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); |
Contributor
There was a problem hiding this comment.
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);
added 7 commits
January 9, 2020 10:12
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 <[email protected]>
1.Using named exports 2. Returning errors from rejects 3. Removed dev artifacts Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>
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 <[email protected]> Pre feedback addressed 1.Using named exports 2. Returning errors from rejects 3. Removed dev artifacts Signed-off-by: Srinaath Ravichandran <[email protected]> Made sure all pacjage locks are removed and spacing between cases Signed-off-by: Srinaath Ravichandran <[email protected]> Untracked Package Locks Signed-off-by: Srinaath Ravichandran <[email protected]> Untracked and deleted package locks Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>
tonyanziano
reviewed
Jan 10, 2020
tonyanziano
reviewed
Jan 10, 2020
tonyanziano
reviewed
Jan 10, 2020
tonyanziano
reviewed
Jan 10, 2020
tonyanziano
reviewed
Jan 10, 2020
tonyanziano
reviewed
Jan 10, 2020
tonyanziano
suggested changes
Jan 10, 2020
Contributor
tonyanziano
left a comment
tonyanziano
left a comment
There was a problem hiding this comment.
Some more minor comments.
srinaath
commented
Jan 10, 2020
Comments tag removed Typos fixed Signed-off-by: Srinaath Ravichandran <[email protected]> Removed unused functions Signed-off-by: Srinaath Ravichandran <[email protected]>
tonyanziano
reviewed
Jan 10, 2020
| return ( | ||
| <> | ||
| <legend> | ||
| Looks like you have hit your free tier limit on connections to your tunnel. These are few solutions. |
Contributor
There was a problem hiding this comment.
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.
tonyanziano
previously approved these changes
Jan 10, 2020
Contributor
tonyanziano
left a comment
tonyanziano
left a comment
There was a problem hiding this comment.
One small comment. Once that's fixed I think this is good to go!
Signed-off-by: Srinaath Ravichandran <[email protected]>
Contributor
Author
Thanks @tonyanziano |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ngrok Debug Console
Added console window to track tunnel errors
Pinned React version
More unit tests coverage in future PR's