Skip to content
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

Initial Apollo Client setup and ability to add/remove/update/get Project Files #1209

Merged
merged 6 commits into from
Oct 9, 2020

Conversation

jk05
Copy link

@jk05 jk05 commented Oct 5, 2020

Browser File IO Feature Branch

  • Displays Project Files specific UI (save button in Editor frame and Sidebar drawer) only when certain params are passed in via Desktop
  • Add in Apollo Client allowing getting/adding/removing/updating Project Files on the file system (via Relate)
  • Adding a project file will currently only be added to the root of a Project in this phase. Getting/Removing/Updating can work at any level in a project for now.

How to test this work

  • Desktop now has latest changes on master branch. Relate has a fix for token verification (on master) but not been released or updated in Desktop.
  • The method I used to test this work right now is to launch dev Browser from Desktop (enabling dev mode). Either as a graph app file url or via the dev mode url input. This will launch Browser from Desktop and allow you to get the params passed to Browser from from the dev console (new URL(location.href).search) and copy them.
  • The params will look something like ?relateApiToken=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpYXQiOjE2MDIxNjk1NTcsImV4cCI6MTYwMjI1NTk1N30.ktiZGi4TXzSSOa1a1WlZD7HlFU-lyVFR3-koFF89v24&relateUrl=http%3A%2F%2F127.0.0.1%3A12111&neo4jDesktopProjectId=project-6d927250-a154-4d3e-9052-a372bd269705&neo4jDesktopGraphAppRootPath=&neo4jDesktopGraphAppId=neo4j-browser-1&neo4jDesktopApiUrl=http%3A%2F%2F127.0.0.1%3A11001&neo4jDesktopGraphAppClientId=cd50b2fd9f5a8704efdf52f7e2aff663065c32b6f77e0b7b27e7f9868da9da80&neo4jDesktopInternalAppId=0bcdeecb-c3f6-4bfb-a2f8-c5e3b5e009b0 . You should be done with Desktop at this point.
  • Now in order to test with latest Relate, pull latest master and run relate web (may be a port conflict if you still run Desktop, so either close or pass in a port var). Then with Browser running in your web browser, paste the params in and make sure to change relateUrl to point to :3000 (Relate). It should look something like ?relateApiToken=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpYXQiOjE2MDIxNjk1NTcsImV4cCI6MTYwMjI1NTk1N30.ktiZGi4TXzSSOa1a1WlZD7HlFU-lyVFR3-koFF89v24&relateUrl=http%3A%2F%2F127.0.0.1%3A3000&neo4jDesktopProjectId=project-6d927250-a154-4d3e-9052-a372bd269705&neo4jDesktopGraphAppRootPath=&neo4jDesktopGraphAppId=neo4j-browser-1&neo4jDesktopApiUrl=http%3A%2F%2F127.0.0.1%3A11001&neo4jDesktopGraphAppClientId=cd50b2fd9f5a8704efdf52f7e2aff663065c32b6f77e0b7b27e7f9868da9da80&neo4jDesktopInternalAppId=0bcdeecb-c3f6-4bfb-a2f8-c5e3b5e009b0
  • You should now see the Project Files specific UI and graphQL calls working correctly. If you remove all the params or specifically any/all of relateApiToken, neo4jDesktopApiUrl or neo4jDesktopProjectId, then then Project Files specifics will not show at all (Apollo client should not cause any problems in this case)
  • Prior UI tweaks have been added in small adjustments for Project Files coming from Relate (prevent folder and file rename and folder deletion) neo4j-devtools/relate-by-ui#7 and updating a file added in updateFile method added to projects neo4j-devtools/relate#199.

Screenshots and Videos:
Screenshot 2020-10-05 at 13 58 26

Video 1 : showing files being created, updated, removed and general UI

Video 2 : network error handling

Video 3 : graphQL error handling

Video 4 : edit file error status

changelog: Add support for storing scripts as project files when in a desktop environment (for desktop versions that support it)

build_scripts/webpack.config.js Outdated Show resolved Hide resolved
@jk05 jk05 marked this pull request as ready for review October 5, 2020 13:09
@jk05 jk05 requested a review from huboneo October 5, 2020 13:09
@jk05 jk05 force-pushed the browser-file-io branch from af97ba1 to b19ee00 Compare October 5, 2020 13:24
Copy link
Contributor

@huboneo huboneo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments from first browse. also please delete package-lock.json

src/browser/modules/Editor/ProjectFilesButton.tsx Outdated Show resolved Hide resolved
src/browser/modules/Editor/ProjectFilesButton.tsx Outdated Show resolved Hide resolved
src/browser/modules/Sidebar/project-files.utils.ts Outdated Show resolved Hide resolved
src/browser/modules/Sidebar/project-files.utils.ts Outdated Show resolved Hide resolved
@huboneo huboneo self-requested a review October 6, 2020 13:46
@jk05 jk05 force-pushed the browser-file-io branch from b19ee00 to 37ed8a0 Compare October 6, 2020 14:13
@jk05 jk05 requested a review from huboneo October 6, 2020 14:15
@jk05 jk05 force-pushed the browser-file-io branch 2 times, most recently from 50d13ee to ffe97dc Compare October 6, 2020 15:15
@jk05
Copy link
Author

jk05 commented Oct 6, 2020

N.B. JS tests for 3.5 are failing for Editor.test - need to look into testing with Apollo or something quick to fix for the time being. Will have to be in another PR.

@jk05 jk05 requested a review from OskarDamkjaer October 7, 2020 07:39
@jk05 jk05 force-pushed the browser-file-io branch from ffe97dc to a0457a0 Compare October 7, 2020 08:29
src/browser/modules/Editor/ActionButtons.tsx Outdated Show resolved Hide resolved
src/browser/modules/Editor/EditorFrame.tsx Outdated Show resolved Hide resolved
isDrawerOpen: boolean
}

type IActiveRelateFile = Omit<IProjectFile, 'downloadToken'>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use the I-prefix in the rest of the project and I don't see the use of it in typescript, so I'd prefer the I-s be omitted

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok fair enough. I like the I prefix as it makes it clear but happy to remove and rename

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The I prefix is for interfaces by convention :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Id like to keep this too. Thoughts @OskarDamkjaer ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same time, I appreciate that the rest of Browser may not use it so you dont want to add it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides, IActiveRelateFile is not even an interface? I think the prefix makes sense in C# but not in typescript, at the very least let's not have a mix of both

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I will remove the I prefix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree here, if it's a type it should not have a prefix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on @HerrEmil comment, we should probably change this eslint rule in relate then and update our code for consistency

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ill add a card for this

src/browser/modules/Sidebar/ProjectFiles.tsx Outdated Show resolved Hide resolved
src/browser/modules/Sidebar/ProjectFiles.tsx Outdated Show resolved Hide resolved
src/browser/modules/Sidebar/ProjectsFilesScripts.tsx Outdated Show resolved Hide resolved
src/browser/modules/Sidebar/ProjectsFilesScripts.tsx Outdated Show resolved Hide resolved
src/browser/modules/Sidebar/Sidebar.tsx Show resolved Hide resolved
src/browser/modules/Sidebar/project-files.utils.ts Outdated Show resolved Hide resolved
src/browser/modules/Sidebar/project-files.utils.ts Outdated Show resolved Hide resolved
@jk05 jk05 force-pushed the browser-file-io branch from a0457a0 to 8438eb4 Compare October 7, 2020 10:24
@jk05 jk05 requested a review from OskarDamkjaer October 7, 2020 10:25
@jk05 jk05 force-pushed the browser-file-io branch 2 times, most recently from 526d04c to e8f1067 Compare October 8, 2020 12:08
@jk05
Copy link
Author

jk05 commented Oct 8, 2020

@huboneo @OskarDamkjaer can you guys check these latest commits:

0dbbcb8
8a5cfd8
e8f1067

These add the ability to work with params passed from Desktop and cleanups some path issues.

@jk05 jk05 force-pushed the browser-file-io branch from e8f1067 to 0a9bccd Compare October 8, 2020 15:27
@jk05 jk05 requested a review from huboneo October 8, 2020 15:28
@jk05 jk05 force-pushed the browser-file-io branch 2 times, most recently from 9544a5a to 20789b5 Compare October 9, 2020 08:54
@jk05 jk05 force-pushed the browser-file-io branch from 20789b5 to f7570c9 Compare October 9, 2020 09:00
width={16}
buttons={buttons}
editorValue={() => this.getEditorValue()}
isRelateAvailable={this.props.isRelateAvailable}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in order to get tests to pass. I will look at additional tests during code freeze.

@jk05 jk05 requested a review from OskarDamkjaer October 9, 2020 10:02
@jk05 jk05 removed the DONT-MERGE label Oct 9, 2020
Copy link
Contributor

@huboneo huboneo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments, but LGTM

relateUrl: props.relateUrl
})
)
const projectFiles = await Promise.all(getProjectFilePromises)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically speaking shadowing, but wont affect logic at this point

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, should name it something else ideally but Ill leave it for now


// when a saved Project Script or Local Cache Script is clicked
// not sure at this point which it could be
bus &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this syntax is harder to read than just having if (!bus) return above

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I do agree, I just kept it in line with how its done in the rest of Browser. I can change if needed.

@jk05 jk05 merged commit 61aeb5b into neo4j:master Oct 9, 2020
@jk05 jk05 deleted the browser-file-io branch October 9, 2020 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants