Add router (connected-react-router)

ryanlaws commented 4 years ago

This adds react-router and supporting packages and:

Integrates them into the root reducer and middleware.
Introduces a URL handler bound to state, which:
- Sets the URL according to the path of the last script loaded into the REPL
- Attempts to load the script in the REPL on page load if the URL contains such a path.
Adds an async action to recursively open the directory hierarchy containing the script.
Adds support utilities to convert between a file path and a URL path.
fetch for REPL endpoints now looks in both / and /maiden before failing.

ryanlaws commented 4 years ago

Oops, I was testing with React's dev server and spaced on 404 handling which is going to require some modifications to the server. I'm a Go noob but I'm investigating. Looks like gin might need to be updated - https://github.com/gin-gonic/gin/pull/1663

ryanlaws commented 4 years ago

Looks like codec also needs either an update or a replace. The former seems cleaner. https://github.com/gin-gonic/gin/issues/1673

ryanlaws commented 4 years ago

OK, I think that oughtta do it.

ryanlaws commented 4 years ago

I missed units.json as well. My fetchJsonFromRoot implementation looks a little hacky - suggestions welcome.

ryanlaws commented 4 years ago

Great feedback! Thank you for taking a look at this! I probably won't be able to dig in right away, but I think using a fragment instead makes a lot of sense as long as it persists to history. If we can avoid messing with the server code, that certainly seems preferable - that definitely felt a little wrong.

ryanlaws commented 4 years ago

in the future we could add / after the resource to allow multiple editor views...?

Hmm, I'm not completely following here. Is <n> just an integer? Is it currently possible to split the editor view or something?

One other thing I tried which didn't work and was possibly confusing was that while browser history seemed to be enabled using the back button in the browser window would show the URL of the previous file I had directly edited but neither the editor itself nor the file tree changed to display the previous file.

Good call. That was super funky and I completely missed it in my testing. I trashed the componentDidMount for page load and made it use state instead and it seems much cleaner to me now.

Let me know if you think this needs more changes. This feels pretty good to me now, but it's quite possible I missed something else. A couple of things you mentioned sound intriguing but weren't completely clear to me. They might make more sense in the morning.

ryanlaws commented 4 years ago

@ngwese I refactored and added some tests and resolved the above conversations. Let me know whether this fits what you had in mind, or still needs work.

ngwese commented 4 years ago

I will take a deeper at the code in a bit but one of the things which I noticed quickly was that something funky is going on with the refresh of the file tree now (I don't think this was in the previous version). It appears as if went switching files where is some code which runs async and tries to refresh the tree which causes the previous expanded portion of the tree to collapse/disappear but the disclosure triangles still indicate it is expanded.

I captured a video but apparently I can't attach it the PR so... http://afofo.com/files/norns/maiden-router-file-tree-glitch.mov

ryanlaws commented 4 years ago

Ah, great, thanks for the video, that is super helpful! Earlier I noticed some similar funkiness when loading scripts in the same directory, which is what the inSameDir and its consumer are meant to address. It makes sense that you'd see that in a sibling directory. That whole expanding thing probably only makes sense to do on page load (from fragment).

I was able to repro the behavior from your video and I've reworked it and as a bonus managed to factor out and remove inSameDir.

ngwese commented 4 years ago

It all looks good and working on my end!

I squashed down a few of the intermediate commits where the router was still using / instead of # in order to avoid some confusion but left the rest of the commit history alone. I did the partial squash and merge outside of GH in such a way that it appears GH no longer associates it with this PR.

Closing out this PR now that the changes have been merged.

ryanlaws commented 4 years ago

Cool, thank you!

monome / maiden

Add router (connected-react-router) #167