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

Browser File IO - add Apollo Client and relate-scripts component in sidebar #1201

Closed
wants to merge 3 commits into from

Conversation

jk05
Copy link

@jk05 jk05 commented Sep 28, 2020

  • NO NEED TO PR AT THE MOMENT. HAVE TO MAKE SEVERAL LARGE CHANGES IN A SEPARATE BRANCH DUE TO CHANGING REQUIREMENTS THAT I WILL MERGE IN AND THEN UPDATE THIS PR
  • I may use this as my main feature branch and just merge into this going forward
  • Add in Apollo Client and Apollo Upload Client
  • Created a Relate Scripts Component to display in the Sidebar, displaying all the Project Files
  • Had to proxy requests to relate-web for now as getting CORS issues when trying to use Relate API url passed from Desktop. Will work on this in an upcoming PR

Small adjustments to the saved-scripts component here: neo4j-devtools/relate-by-ui#7, which are visible in the screenshot. Remove export/new folder and editing folder/file names in the sidebar

@@ -103,6 +103,10 @@ module.exports = {
host: '0.0.0.0',
port: 8080,
disableHostCheck: true,
hot: !helpers.isProduction
hot: !helpers.isProduction,
proxy: {
Copy link
Author

Choose a reason for hiding this comment

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

proxy for now until we use params passed from Desktop and a potential CORS issue that was being experienced

const filePath = path.join(directory, favorite.name)
return removeFavorite({
variables: removeProjectFileMutationVars(filePath),
update: (cache, { data: { removeProjectFile } }) => {
Copy link
Author

Choose a reason for hiding this comment

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

update cache

}) => ({
id: uuid.v4(),
name,
path: directory.startsWith('.') ? SLASH : `${SLASH}${directory}`, // @todo: need to look into this
Copy link
Author

Choose a reason for hiding this comment

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

couldnt get this working with . which is the root returned from Relate. Had to use SLASH for now just to get it to show, will look into later as its not a blocking issue

remote
.request('GET', `/files/${token}/${name}`)
.then(body => body.text())
.catch(e => console.log('++err', e)) // ?
Copy link
Author

Choose a reason for hiding this comment

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

need to look into handling errors and what to do in next few PRs

Copy link
Author

Choose a reason for hiding this comment

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

Any suggestions here are welcome


const MyScriptsComponent = props => {
// @todo: handling loading and error?? Pass to MyScripts??
const { loading, error, data, refetch } = useQuery(GET_PROJECT_FILES, {
Copy link
Author

Choose a reason for hiding this comment

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

error or loading handling suggestions welcome here

Copy link
Contributor

Choose a reason for hiding this comment

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

What can cause errors? Does it only error if the connection is broken? Just thinking how serious the errors are

Copy link
Author

Choose a reason for hiding this comment

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

network error like when connection is broken I suppose. Or errors from the graphQL endpoint

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm.. perhaps it's worth having an errorbanner like db not available but for network? For errors from the end point I'm not sure.. We want them in sentry eventually at least

Copy link
Author

Choose a reason for hiding this comment

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

yeah the banner makes sense. I will look into that, I didnt look in to the error banners

const directory = favorite.path.substring(1) // @todo: adding in SLASH to path
const filePath = path.join(directory, favorite.name)
return removeFavorite({
variables: removeProjectFileMutationVars(filePath),
Copy link
Author

Choose a reason for hiding this comment

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

need to think of error handling here too

const isMountedRef = useRef(false)

useEffect(() => {
isMountedRef.current = true
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't need to be a useRef, a closure is enough. I changed it in my pr :)

@jk05 jk05 closed this Oct 5, 2020
@jk05 jk05 deleted the browser-file-io branch October 5, 2020 12:46
@jk05
Copy link
Author

jk05 commented Oct 5, 2020

closed in favour of #1209

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.

2 participants