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

feat: guide the user when no api found #715

Merged
merged 20 commits into from
Aug 24, 2018
Merged

Conversation

olizilla
Copy link
Member

@olizilla olizilla commented Jul 6, 2018

Inform the user when we can't connect to the API and give them the option to set a custom api address.

screenshot 2018-07-06 17 25 51

Early WIP on #711

License: MIT
Signed-off-by: Oli Evans oli@tableflip.io

@hacdias
Copy link
Member

hacdias commented Aug 3, 2018

@hacdias
Copy link
Member

hacdias commented Aug 3, 2018

@olizilla I've made some changes. Please take a look to ipfs-redux-bundle.

  • If we can't connect to an instance, it redirects to /welcome.
  • It won't let you open any other page if there isn't a valid IPFS instance.
    • This makes our navigation tests fail on CI because there isn't an IPFS instance running. Suggestions? We could start a js-ipfs instance so it connects and does everything else.
  • Cache! So, you set an API address, reload the page, and it's still the same address.
  • You can always go to /welcome and change the API url.

@hacdias hacdias force-pushed the feat/revamp/no-api-found branch 2 times, most recently from 18f18b9 to de276b3 Compare August 4, 2018 09:52
olizilla and others added 11 commits August 20, 2018 09:42
License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
Copy link
Member Author

@olizilla olizilla left a comment

Choose a reason for hiding this comment

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

Redirecting the url when ipfsReady is false feels like something that we should set up in redux.

We should catch the condition that the welcome page bundle has loaded, but IPFS isn't ready and there is no error. It can just show a loading spinner on the welcome in that case.

@hacdias
Copy link
Member

hacdias commented Aug 21, 2018

@olizilla

Redirecting the url when ipfsReady is false feels like something that we should set up in redux.

It is on redirects bundle.

We should catch the condition that the welcome page bundle has loaded, but IPFS isn't ready and there is no error. It can just show a loading spinner on the welcome in that case.

Done.

When failed to connect.

image

After being successfully connected

image

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member

hacdias commented Aug 21, 2018

@olizilla btw changed 'Your IPFS is running!' to 'Your IPFS daemon is running!'

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias hacdias changed the title [wip] guide the user when no api found feat: guide the user when no api found Aug 21, 2018
@olizilla
Copy link
Member Author

@hacdias To complete this challenge please appease the CI gods. The tests should pass and the test coverage should not go down. Of note the navigation test that is failing was really a place holder for more exciting e2e tests. Do you accept this mission?

@hacdias
Copy link
Member

hacdias commented Aug 21, 2018

For this PR I can add tests with and without a node running. We can then think about more tests haha. Sounds good @olizilla?

@olizilla
Copy link
Member Author

@hacdias 💯

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member

hacdias commented Aug 21, 2018

@olizilla could you take a look at the code? I'm getting Protocol error (Runtime.callFunctionOn): Target closed.. I found some solutions online related to the memory, but they don't seem to work. I've also tried connecting with the Address instead of setting window.ipfs but the error persists.

// Save 3s per run by using the initial page rather than creating a new one!
// const page = await browser.newPage()
const page = (await browser.pages())[0]

await page.evaluate(() => {
window.ipfs = node
Copy link
Member Author

Choose a reason for hiding this comment

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

@hacdias ok so, this is a rad idea, but I don't think it'll work. With the e2e tests things get complicated, as the test is running in jest in node, and firing up a browser instance with it's own seperate runtime. I don't think we'll ever be able to create an ipfs instance in a node runtime and smoosh it into a browser without very weird things happening.

Much more minor, the api for page.evaluate requires you to pass any args you want to hand to the page, and they get magically transferred as args to the callback function when it is executed in the context of the page instance. Even so, like going through a black hole, an ipfs instance won't survive the trip.

One solution is to create a fake window.ipfs object with an id function that resolves with a test identity info, and then check for that. We don't need to test that js-ipfs works in the webui tests, so it's ok to stub it out.

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member

hacdias commented Aug 21, 2018

@olizilla made some updates and mocking but it seems to fail here: 6a0f41d#diff-0238dfb49a06ba796757bff9fcff0faaR41

There might be something on the files page that blocks the rendering? I can't see what it might be though. I tried adding empty functions for files.ls and files.stat but it didn't work.

@olizilla
Copy link
Member Author

@hacdias i'll take a look at this now

@hacdias
Copy link
Member

hacdias commented Aug 24, 2018

Thank you @olizilla

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
remove selectApiUrl from config bundle as it's better got from the ipfs bundle

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants