-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Multi proto file support #3006
Multi proto file support #3006
Conversation
…w deleting a proto directory
…proto manager tests
@@ -44,7 +44,7 @@ export function hideAllModals() { | |||
} | |||
|
|||
function _getModal(modalCls) { | |||
const m = modals[modalCls.name]; | |||
const m = modals[modalCls.name || modalCls.WrappedComponent?.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.
Needed because the ProtoFileModal is wrapped by Redux's connect
HoC
packages/insomnia-app/app/ui/components/proto-file/proto-directory-list-item.js
Outdated
Show resolved
Hide resolved
@@ -36,6 +36,7 @@ | |||
"\\.(css|less|png)$": "<rootDir>/__mocks__/dummy.js", | |||
"^worker-loader!": "<rootDir>/__mocks__/dummy.js" | |||
}, | |||
"modulePathIgnorePatterns": ["<rootDir>/network/.*/__mocks__"], |
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.
Workaround due to an apparent bug in Jest: jestjs/jest#2070 (comment)
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.
Added screenshots of all tests to allow for easier review
@@ -0,0 +1,93 @@ | |||
// @flow |
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.
@@ -0,0 +1,54 @@ | |||
// @flow |
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.
@@ -0,0 +1,258 @@ | |||
// @flow |
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.
@@ -0,0 +1,283 @@ | |||
// @flow |
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.
@@ -0,0 +1,61 @@ | |||
// @flow |
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.
@@ -0,0 +1,183 @@ | |||
// @flow |
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.
…-import-proto-file-directory
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.
Very nice flow and provisioning! Despite being an MVP I feel that we should represent structure with more visual reenforcement. Appropriately, nested parents don't receive a hover yet, aside from tabs, we are visually treating them as doc line items (bordered row). We can leverage the :before selector on parents to visually stub something like the following leveraging your existing hook for padding/indent treatment:
Thoughts?
@sonicyeti I quite like the first option! Only concern is that right now there's no expand/collapse function in there, so could it be misleading? Could also use the folder icon (consistent with the sidebar) |
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.
Looks good! 🎉
A few questions concerning the loading spinner state when importing a directory, don't think they're blocking for this PR:
When importing a large directory that takes a while, after re-opening the modal and pressing import directory again, nothing happens, it'll just keep spinning (presumably until finished, the directory I was testing with takes tens of minutes to load, so I didn't verify), not sure if that should be handled differently?
You can also delete the new directory while it's still importing, and the spinner will keep running, not sure what state the DB ends up in there, if there's potential issues?
message: `No .proto files were found under ${filePath}.`, | ||
}); | ||
} | ||
// TODO: validate all of the imported proto files |
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.
Did this get missed, or are you thinking we'll do this at a later stage?
Noticed that I got no error when I tried to load a directory that had an invalid proto file within, the files showed up, but selecting them would just not update the selected methods for the request.
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 seems like an important enough case to do as part of MVP. At the bare minimum displaying a warning or error out completely with a message stating that there is an invalid protofile.
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.
// Loop and read all entries | ||
const parsePromises: Array<Promise<boolean>> = entries.map(entry => { | ||
const fullEntryPath = path.resolve(dirPath, entry.name); | ||
return entry.isDirectory() |
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.
Don't think we necessarily need to do this in this PR, but we probably want to support symlinks for both files and directories here as well (isSymbolicLink
) eventually, because it's pretty common to symlink folders from various parts of a repository into a main protofile folder, and just pass that to protoc
or whatever tools you are using to consume your protofiles/
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.
Yes that's a great point and we should support it. I think it would be best to create an issue for it in Linear (if not already there) so we don't skip it and can prioritize it - INS-432
Yeah this is a great point; we can disable UI interactions but in order to keep the state in between closing and re-opening the modal, we'd need move the loading state into redux (or context). This isn't a common pattern, although it should be - many of the git/beta sync loading states are within the drop-down, and re-opening the drop-down breaks the state as well. I'll look to address the second issue (deleting while adding INS-433), but will likely not fix the first one (large directory INS-434) right now. 🤔 |
@sonicyeti I have made the styling changes in #3023 - INS-430 |
…-import-proto-file-directory
This PR introduces multiple proto files support in the form of directory uploads, as well as some refactoring in order to increase the test coverage of existing and new logic.
It is a fairly large PR in terms of file- and line-count, many of which are unit tests. Here is a general breakdown of the various aspects of this PR.
New database model
ProtoDirectory
is a new database model that has been created, and it behaves in a manner similar toRequestGroup
.Proto file management business logic
Previously, much of the proto file management business logic was written within
ProtoFilesModal
. This is tricky to test because logic is intertwined within a React component. The business logic was extracted tonetwork/grpc/proto-manager
. Function were then added for directoriesaddDirectory
,deleteDirectory
, as required by this feature. All of this logic is now unit tested, and the modal component greatly simplified. (In the future, this will also allow for adding the proto files and directories list alongside certificates, in workspace settings).UI
Changes to the UI are primarily in the proto files list, which now need to show indents, read-only states and directories. This was achieved through extending the
ListGroupItem
component with indent/hover/select state logic (available in the storybook for this PR).A generic
ProtoListItem
component, extendsListGroupItem
and sets a common height and icon font size, specificly for the proto file management view.Lastly,
ProtoFileListItem
andProtoDirectoryListItem
both extendProtoListItem
and include specific logic for each view, indenting and setting read-only states accordingly. (Only individual proto files at the root level can be renamed/reuploaded/deleted, and only directories at the root level can be deleted).This is the overall component hierarchy of the proto file list items as described above:
ListGroupItem
(ininsomnia-components
)ProtoListItem
(ininsomnia-app
)ProtoFileListItem
ProtoDirectoryListItem
Upload a directory
When ingesting a directory, we follow a recursive algorithm.
For a given directory,
ProtoDirectory
entity under the parent,.proto
extension, create aProtoFile
entity with the file name and contents,This algorithm starts with the workspace being the first parent.
A step also exists to sanity check the data. For example, if a directory is loaded but no
.proto
file exists in it or in a sub-directory, then that directory is of no use to us, so the database entry is deleted, keeping the database clean. There is a unit test iningest-proto-directory.test.js
using a fixture containing a sub-directory like this.Parsing a proto file
When parsing a proto file for making requests, we must treat nested and individual files slightly differently. If a proto file has been selected, which is part of a directory tree, we must write the entire tree to the file-system, in order for the proto loader to successfully import relative paths within the proto file.
We do this by determining whether the selected proto file has any ancestor of type
ProtoDirectory
in the database. If so, we find that root directory, and then find all descendants of typeProtoDirectory
orProtoFile
, and recursively write the tree onto the filesystem.An individual file is written under
<tempDir>/<id>.<modified>.proto
to ensure we don't write more than necessary, but always have the latest contents as well. For directories, we follow a similar pattern, however the files and folders should keep their original names. Therefore, we use<tempDir>/<id>.<modified>/folder/root.proto
, where theid
andmodified
time are properties of the root folder.Screenshots
Nothing uploaded
Individual file & directory uploaded
The directory uploaded exists at
packages/insomnia-app/app/network/grpc/__fixtures__/library
in this PRroot.proto
selectedtime.proto
selectedhello.proto
selectedIt works! 🎉
Known limitations
Closes #2955