-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Updates #98
base: dev
Are you sure you want to change the base?
Updates #98
Conversation
joepav
commented
Jun 27, 2019
•
edited
Loading
edited
- Replaced websockets with IPC
- Ability to save to file system
- Ability to load saved data from file system
- Specify PORT and activate server
merging Dev into master
Merging Dev into Master
Merging Dev into Master
Rebranded the tool
Dev into master
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.
Love the streamlining of using the IPC modules to handle the server—just a few comments/suggestions and one potentially major bug.
handleRequest(request, response); | ||
}); | ||
|
||
ipcMain.on('activate_server', (event, arg) => { |
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 work getting rid of our socket.io dependency 🙌
/> | ||
); | ||
|
||
socket.on('post_received', (postedData) => { | ||
ipcRenderer.on('saving_or_opening_error', (error, postedData) => { | ||
alert(JSON.stringify(postedData || error)); |
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.
if (validData) mainWindow.webContents.send('opened_file', parsedJSON); | ||
else { | ||
mainWindow.webContents.send('saving_or_opening_error', | ||
'Error opening: Invalid data structure'); |
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.
See note below: I think we can clean up errors for invalid JSON (like this error for valid JSON in an invalid structure)
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.
This error could also probably be more helpful:
Suggestion
Error: This file doesn’t follow the expected structure—try importing JSON that was originally saved from Interspect
} = props; | ||
|
||
const turnServerOn = (e) => { | ||
const port = Number(document.querySelector('#portNumInput').value); |
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.
Not sure where to put this comment, but I think it’d be great to implement @Donaldblanc’s suggestions of randomizing the port here to one not in use. The Node.js docs here indicate:
If port is omitted or is 0, the operating system will assign an arbitrary unused port, which can be retrieved by using server.address().port after the 'listening' event has been emitted.
<p>To get started, send a request to any of your microservices <br/>or post JSON to http://localhost:3001/posturl</p> | ||
<p>To get started, send a request to any of your microservices</p> | ||
{!serverOn | ||
&& <Button enabled onClick={turnServerOn}> Turn HTTP request listening on </Button> } |
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.
@joepav I feel like you already mentioned shortening up this button. If we can randomize the port, I think we could simplify the UI to something like this:
Interested to here your thoughts!
<p>{(serverOn | ||
? `or post JSON to http://localhost:${listeningPort}/` | ||
: 'to post JSON directly to Interspect')}</p> | ||
<Button enabled onClick={openFile}> Open Saved Tests </Button> |
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.
I think it can wait, but for platform consistency, we might want to consider adding an “Open…” option to the File menu as well
@@ -72,6 +74,10 @@ const MockupsPanel = (props) => { | |||
setCreateTestIndex(value); | |||
} | |||
|
|||
function saveFile(e) { | |||
ipcRenderer.send('save_file', JSON.stringify({ data, tests })); |
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.
Found a potential bug here 🐞
Steps to reproduce:
- Get source data (I used https://jsonplaceholder.typicode.com/users)
- Click Save Work as JSON
- Save as “Untitled” to a path where “Untitled” already exists
- A dialog should appear asking if you want to replace the file
- If you choose to replace, you should get an infinite error loop—must force quit app to get rid of it.
The error is: {"code": "EISDIR","errno":-21,"path":"/<file path>","syscall":"open"}
Again, this caused an infinite error loop on my machine—there was no way to get rid of it or use the app until I force quit the process.
@@ -96,6 +102,8 @@ const MockupsPanel = (props) => { | |||
> | |||
{options} | |||
</Select> | |||
<br /> | |||
<Button enabled onClick={saveFile}>Save Work as JSON</Button> |
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.
Similar to opening a file, it’d be great to get this option in the file menu as well. I will also take a crack at some UI simplification but focusing on functionality for now 👍
|
||
const Panels = () => { | ||
const [activePanel, setActivePanel] = useState('source'); | ||
const [data, setData] = useState(undefined); | ||
const [getFetchTimes, setGetFetchTimes] = useState([]); | ||
const [postFetchTimes, setPostFetchTimes] = useState([]); | ||
const [hContentType, setContentType] = useState(''); | ||
const [listeningPort, setListeningPort] = useState(3002); |
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.
Could also maybe use 0
here to randomize port
// status: initial value of '' }; | ||
// status: initial value of '', | ||
// name: initial value of 'Test #__' | ||
// diff: set iff subset, then it's subset key name |
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.
Typo?