-
Notifications
You must be signed in to change notification settings - Fork 6k
feat: persist route query to local #1920
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
Conversation
Provide a way for the shell script running in the docker container to get the url query.
Thanks for the contribution! Could you expound on the use case? |
Thank you for your reply. I did not find a solution to the problem, so I think this may be a new feature. |
Interesting, thanks for the additional details. We already have a file that contains the last visited path in |
Of course, it will help a lot!
…------------------ Original ------------------
From: Asher <[email protected]>
Date: Wed,Jul 29,2020 11:41 PM
To: cdr/code-server <[email protected]>
Cc: fxxjdedd <[email protected]>, Author <[email protected]>
Subject: Re: [cdr/code-server] feat: persist route query to local (#1920)
Interesting, thanks for the additional details. We already have a file that contains the last visited path in ~/.local/share/code-server/coder.json (just a bit further down in getRoot). Would it work for your use case to add the query variables to the lastVisited object in that file instead?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Awesome, let's go that route then. |
I added a shallow parameter, because the query should not be extends, but should be replaced directly.
I made some changes and it works fine locally. |
emmm, I have some different opinions:
Although await is clearer, but it will blocks all the code below, but promise.then will not.
On the other hand, if they are written together, the query will be updated using extends, so that when the query becomes empty, the data in the settings will not be updated correctly
…------------------ Original ------------------
From: Asher <[email protected]>
Date: Fri,Jul 31,2020 0:16 AM
To: cdr/code-server <[email protected]>
Cc: fxxjdedd <[email protected]>, Author <[email protected]>
Subject: Re: [cdr/code-server] feat: persist route query to local (#1920)
@code-asher requested changes on this pull request.
In src/node/app/vscode.ts:
> if (startPath) { - settings.write({ - lastVisited: startPath, - }) + promise = settings.write({ lastVisited: startPath })
Ah oops, this is a bug on my part because I forgot to await the promise. I think it might be a bit cleaner to use await.
We can also update both values at the same time since the default value of startPath is the old value that was in the settings.
In src/node/settings.ts:
> @@ -55,6 +58,7 @@ export interface CoderSettings extends UpdateSettings { url: string workspace: boolean } + query: Route["query"]
I didn't know you could do this. Very cool!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
That's fair, let's leave out the We can pass |
In fact I'm thinking about removing |
cool, let‘s have a try!
…------------------ Original ------------------
From: Asher <[email protected]>
Date: Fri,Jul 31,2020 0:42 AM
To: cdr/code-server <[email protected]>
Cc: fxxjdedd <[email protected]>, Author <[email protected]>
Subject: Re: [cdr/code-server] feat: persist route query to local (#1920)
In fact I'm thinking about removing extends entirely because I'm not sure we even need a recursive merge.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
In addition, the `settings.write` method now uses shallow merge by default
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.
Awesome. Thanks for the contribution!
Provide a way for the shell script running in the docker container to get the url query.