-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add router (connected-react-router) #167
Conversation
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 |
Looks like |
OK, I think that oughtta do it. |
I missed |
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.
All in all thanks for pulling this together! I had wondered about react-router
but hadn't gathered the courage to try and incorporate it.
Broad strokes if you don't have any objections I think this might be cleaner in the long run if we used fragments in the URLs so it was more clear what should is being handled by the client side router and what was being handled on the backend. My hope would that this would also eliminate the need for fallback logic like is encoded in fetchJsonFromRoot(...)
.
As to URL syntax I'd like to suggest:
- <maiden app root>#
- edit/<path to resource> -- in the future we could add /<n> after the resource to allow multiple editor views...?
- [future] repl/... -- for repl related routes
- [future] installed/... -- for installed package related routes
- [future] packages/... -- etc.
...the fragment syntax should (hopefully) eliminate the need for changes in server.go
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.
Regardless I'm excited by this change as it will be useful in finishing off the project management UI.
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. |
This adds react-router and supporting packages and integrates them into the root reducer and middleware. It also introduces a URL handler bound to state, which sets the URL according to the path of the last script loaded into the REPL, and attempts to load the script in the REPL on page load if the URL contains such a path. An async action is also added to recursively open the directory hierarchy containing the script. Some support utilities are added to convert between a file path and a URL path. Finally, the fetch for REPL endpoints now looks in both / and /maiden before failing.
- Use URL fragments (hashes) for path - Revert favicon change 8bf0fef - Revert changes, export API_PATH in api.js - Fix error (React stack trace) on page load without path - Reorganize and clean up
Hmm, I'm not completely following here. Is
Good call. That was super funky and I completely missed it in my testing. I trashed the 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. |
- Refactor url-utils to use existing static API functions - Clean up url-utils, removing unused exports - Undo residual changes to api and utils - Add url-utils tests - Add api test as requested by existing TODO - Add test environment check to avoid workspace test failure
@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. |
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... |
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.
I agree this does look much cleaner and thank you for adding tests for various bits! History looks like it is working well now.
I think the only remaining bit is the regression around the tree collapsing and redundant repainting due to what visually looks like possibly several calls to get node children in quick succession.
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 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 |
Expand behavior caused "flashing" when loading file in another directory. This is refactored to check whether a path is loaded into the URL handler (which should always be the case except on initial page load) and whether the path matches the one already loaded. inSameDir is factored out, no longer used, and removed.
It all looks good and working on my end! I squashed down a few of the intermediate commits where the router was still using Closing out this PR now that the changes have been merged. |
Cool, thank you! |
This adds react-router and supporting packages and:
async
action to recursively open the directory hierarchy containing the script.fetch
for REPL endpoints now looks in both/
and/maiden
before failing.