-
Notifications
You must be signed in to change notification settings - Fork 321
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
Cloud/desktop mode switcher #6448
Conversation
QA 🟢 : I QAed yestarday the part of running local/cloud projects, switching between them and doing a bookclubs. Problems with project manager connection I can see was solved today with lazy initialisation. Debug scences are also working but one bug remains. When you run: cc @wdanilo |
marking as draft to do a major refactor |
// ================= | ||
|
||
const IDE_CDN_URL = 'https://ensocdn.s3.us-west-1.amazonaws.com/ide' | ||
const FALLBACK_VERSION = '2023.1.1-nightly.2023.4.13' |
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.
since for now cloud always is sure that it is ran on desktop we should I think fallback to IDE provided with the desktop..
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 think this code shouldn't ever be executed on desktop - the service worker should only be registered when doing npm run watch-dashboard
, see:
IS_DEV_MODE: JSON.stringify(args.devMode), |
enso/app/ide-desktop/lib/dashboard/src/index.tsx
Lines 22 to 27 in ae94a9f
if (IS_DEV_MODE) { | |
new EventSource(ESBUILD_PATH).addEventListener(ESBUILD_EVENT_NAME, () => { | |
location.reload() | |
}) | |
void navigator.serviceWorker.register(SERVICE_WORKER_PATH) | |
} |
content
does have IS_DEV_MODE
as well, but it shouldn't affect this one as dashboard build happens in a separate esbuild instance
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.
if you think it's a good idea to remove it though (since cloud frontend isn't the focus) that's easy enough too
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.
- There is no documentation what
FALLBACK_VERSION
is, so I can't really tell what to do with it until I understand what it is and how it is meant to be used. - If something is cloud-release-specific, it should have that written in the name explicitly.
- cloud release is not our focus, but we can't break it. We need to test it constantly and be sure it works.
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.
it's not possible to make opening the IDE work with cloud frontend because no index.js.gz
has runApp
(partly since it's not merged in yet). maybe we can do something like (window.runApp ?? window.enso.main)
instead?
(or we can just leave it as-is and just stop supporting old versions, which to be fair isn't a bad idea since it won't be possible for new users to create the old versions anyway)
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.
so assuming runApp
polluting the window is the main issue, the current solution i'd assume would be something like this:
const runApp = platform === platformModule.Platform.desktop ?
appRunner.runApp :
window.enso.main;
thoughts/suggestions?
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.
isn't a problem for cloud, but assuming
window.enso.main
is the way to go here, something else must used for the de
I don't understand these points.
window.enso.main
was intended to work and was working fine for long time.- What is the problem with authentication flow on the desktop version?
- Your last sentence states "something else must used for the desktop backend" - why? I do not understand why we can't have one single entry point here. Inside of this entry point you are checking whether the app is run in cloud or not, so why we can't have just one entry point to start the app?
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.
so assuming
runApp
polluting the window is the main issue, the current solution i'd assume would be something like this:const runApp = platform === platformModule.Platform.desktop ? appRunner.runApp : window.enso.main;thoughts/suggestions?
What is appRunner
? Is it another global thing? Why can't we just have window.enso.main
or window.enso.run
(which would be even better than main
).
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.
- it does still work, but not on desktop IDE (which is why i changed it to how it is currently)
- sorry, bad wording - there is no problem with the authentication flow, the issue on the desktop IDE is that calling
window.enso.main
from the IDE React component runs the authentication flow instead of the actual IDE window.enso.main
is the entry point into the entire application, so i don't think the component that starts up the IDE should be running the application entry point
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.
appRunner
is passed into authentication.run()
(the React frontend which includes authentication and the dashboard), in order to avoid needing a global.
the React entrypoint (App
) then passes it down to the Ide
component where it is needed
just tried |
const rootElement = document.getElementById(IDE_ELEMENT_ID) | ||
if (!rootElement) { | ||
logger.error(`The root element (the element with ID '${IDE_ELEMENT_ID}') was not found.`) | ||
} else { | ||
while (rootElement.firstChild) { | ||
rootElement.removeChild(rootElement.firstChild) | ||
} | ||
} |
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.
There is not explanation why this is being done. It looks like we are removing all children from root - why? Please refactor that to a well-named function and provide in the code explanation what cases does it cover. Otherwise, it would be easy to refactor that in the future and break it not knowing the cases.
let currentAppInstance: app.App | null = new app.App({ | ||
config: { | ||
loader: { | ||
wasmUrl: 'pkg-opt.wasm', | ||
jsUrl: 'pkg.js', | ||
assetsUrl: 'dynamic-assets', | ||
}, | ||
}, | ||
configOptions: contentConfig.OPTIONS, | ||
packageInfo: { | ||
version: BUILD_INFO.version, | ||
engineVersion: BUILD_INFO.engineVersion, | ||
}, | ||
}) |
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.
earlier we've been creating new App in the main
function. This was done on purpose, as creating global variables in the initialization time is rather a bad design. I'd like to get back to how it was done. Also, the comment above this line brings more questions than answers:
- Content is a cloud/desktop entry point, so what are command line arguments here?
- What does it mean that it "must be run at least once"? Why it must be? What will happen if its not?
- Why this comment even matter here? It tells that something must be done, but does not explain what is the purpose.
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.
This refactor really helped us with running new Enso App instance on openning project and dropping old thigns. If you would like I could jump on a call to talk about keeping new state.
As for 1-3 points please @somebody1234 as I am also not sure why we create a global here
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.
- the IDE appears to get configuration options from the URL query parameters; i believe neither content nor the frontend control this?
new app.App
mutatescontentConfig.OPTIONS
, which is required otherwise we cannot check whether to open the authentication ui and whether to show the dashboard- the comment explains why this is needed
(note that this isn't actually a global variable accessible to the window because it is a module script. however, i think we can leave it alone and just not assign it to a variable, since it never runs the IDE so we don't have to do any cleanup)
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.
- That's right. So these are not "command line arguments", as there is no "command line" when it is run in the cloud :)
- So the arguments must be parsed, the whole application does not have to be run, right? Running the whole app just to get arguments seems like an overkill.
- I did not understand the comment, tbh.
- Regarding global variables - I understand that these are not window-global variables, but they are module-level global variables and we are trying not to use them this way. Everything should be part of a function and in a module we can call a function to start the evaluation, but everything else should be just nicely refactored to functions.
Anyway, I believe we should refactor it. Keeping the code well-organized is really important to be able to maintain it. If there will be any problems with its refactoring, I'd love to jump on a call.
function tryStopEnso() { | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call | ||
currentAppInstance?.wasm?.drop?.() | ||
} |
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.
- We are not hardcoding product name in function names.
- If this is "try stopping" then when it can fail? There are no docs nor explanation about it.
const config = Object.assign( | ||
{ | ||
loader: { | ||
wasmUrl: 'pkg-opt.wasm', | ||
jsUrl: 'pkg.js', | ||
assetsUrl: 'dynamic-assets', | ||
}, | ||
}) | ||
}, |
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.
- Exactly the same code is above.
- These are the defaults, right? If you don't provide them, they should be the same, am I correct?
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.
- okay, i can extract it out to a function
- seems like they aren't the defaults - defaults are below:
enso/lib/rust/ensogl/pack/js/src/runner/config.json
Lines 17 to 31 in 9a0f739
"wasmUrl": { "value": "pkg.wasm", "description": "The URL of the WASM pkg file generated by ensogl-pack.", "primary": false }, "jsUrl": { "value": "pkg.js", "description": "The URL of the JS pkg file generated by ensogl-pack.", "primary": false }, "assetsUrl": { "value": "assets", "description": "The URL of the dynamic assets directory.", "primary": false },
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.
actually re: 1. the first has been somewhat changed, can be changed even further but as of now "exactly the same code" no longer applies so i'm counting this as done, if something else should be done about this then it can always be brought up in a next review?
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.
- Hmmm, these names are very similar, but a little bit different. This is surprising to me - are we using defaults anywhere? I remember that when I was coding that part, defaults were used in the Electron app. If the names changed, in the Electron app, the defaults should change as well.
currentAppInstance = new app.App({ | ||
config, | ||
configOptions: contentConfig.OPTIONS, | ||
packageInfo: { | ||
version: BUILD_INFO.version, | ||
engineVersion: BUILD_INFO.engineVersion, | ||
}, | ||
}) |
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.
We are creating the app again here. Earlier it was created as a global variable. This looks like a really bad design and something logically is broken here.
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.
This method runEnso
is used when we open projects from the list. We then need to drop
old one and reinitialise the new one with different options (like project name and it case of cloud with different loader options. This is why we need to make a new App here
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.
Paweł, I was talking about the fact that in the code we have 2 places where the app is created. There is never need to copy-paste the same code multiple times. After the recent E-Hern changes, this is no longer the case, but we are still using new app.App({...})
to load configuration, which is extremely hacky.
setTimeout(() => { | ||
clearInterval(handle) | ||
reject() | ||
}, STOP_TRYING_AFTER) | ||
}) |
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.
Is this affecting debug scenes performance? We need to have baremetal performance there without any background checks / connections. If not, how it is done and where in the code we have information about it / checks that for sure debug scenes are not affected by it?
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.
ProjectManager
is (now) lazily initialized so it shouldn't be loaded if the dashboard isn't loaded
what this does is try to open a websocket once per second for up to 10 seconds until it succeeds. not sure how long it takes for new WebSocket
to run but it shouldn't take more than a few ms of cpu time (so not counting io time)
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.
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.
@somebody1234 10 seconds seems like a long time. If the engine started, I understand there might be 10 seconds delay before we try to connect to it, is that correct? If so, this should be 1 second max.
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.
@wdanilo no, its trying to reconnect every 1s for 10 seconds.. meaing it will fail after 10 tries of reconnecting
@@ -0,0 +1,43 @@ | |||
/** @file */ |
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.
no documentation
|
||
import * as cloudService from '../dashboard/cloudService' | ||
import * as localService from '../dashboard/localService' | ||
|
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.
no sections
export function useBackend() { | ||
const { backend } = react.useContext(BackendContext) | ||
return { backend } | ||
} | ||
|
||
export function useSetBackend() { | ||
const { setBackend } = react.useContext(BackendContext) | ||
return { setBackend } |
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.
these functions should either have better names or docs. I do not understand what function useBackend
does (it looks like it does not spawn backend). Even more, I don't understand what useSetBakcend
does.
// ================= | ||
|
||
const IDE_CDN_URL = 'https://ensocdn.s3.us-west-1.amazonaws.com/ide' | ||
const FALLBACK_VERSION = '2023.1.1-nightly.2023.4.13' |
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.
- There is no documentation what
FALLBACK_VERSION
is, so I can't really tell what to do with it until I understand what it is and how it is meant to be used. - If something is cloud-release-specific, it should have that written in the name explicitly.
- cloud release is not our focus, but we can't break it. We need to test it constantly and be sure it works.
if (!rootElement) { | ||
logger.error(`The root element (the element with ID '${IDE_ELEMENT_ID}') was not found.`) | ||
} else { | ||
while (rootElement.firstChild) { |
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.
is it still needed at all? I thought we spoke that #6365 PR will remove div.scene
with all elements? What other elements we are cleaning here?
engineVersion: BUILD_INFO.engineVersion, | ||
// `new app.App` must be run at least once so that the command line arguments are applied to | ||
// `configOptions.OPTIONS`. | ||
let currentAppInstance: app.App | null = new app.App({ |
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.
whats the point of this global? I though maybe we are using it to support old IDE with authentication but onAuthenticated
uses runEnso
. The same if I want to skip auth and new dashboard we also use runEnso
so both case reinitialise the App which looks like bit of a waste
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 have removed it and looks like build works fine
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.
when i added it back, it was because contentConfig.OPTIONS
was never changed from defaults, so the check for whether we should start auth/dashboard doesn't succeed no matter what command line arguments are passed
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.
ok so re-checked.. When this global is removed it seems that authentication
and feature-preview.new-dashboard
flags have default values even if we pass them in command line..
old IDE and debug scenes works so startup.entry
behaves normally, as other flags
@wdanilo this one is tricky.. do you have idea why the contentConfig.OPTIONS
is beahving like that? I see the this.config.loadAll([opts?.config, host.urlParams()])
so maybe we should extract this logic from the App?
still need to switch |
actually never mind on switching to |
not sure if everything's fixed, but they should all be addressed at least |
Unable to perform QA on M2 Mac:
|
// The `any` is unavoidable as `App.wasm` is typed as `any`. | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call |
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.
This comment is not valid anymore. The code below does not access App.wasm
. Please check if the lint disabling is needed.
} | ||
|
||
main() { | ||
contentConfig.OPTIONS.loadAll([app.urlParams()]) |
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.
This is just FYI @somebody1234 as you've been writing about it earlier. The fact that loadAll
mutates the options rather than returning a new object containing them is wrong and it's so because when I was writing that I was unable to code that correctly in TS. However, some day we shiould change it to an immutable API.
const isOpeningIde = | ||
contentConfig.OPTIONS.groups.startup.options.entry.value === | ||
contentConfig.OPTIONS.groups.startup.options.entry.default | ||
if ((isUsingAuthentication || isUsingNewDashboard) && isOpeningIde) { |
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 think we should not use "Ide" word, as it means different things for different people. I think we could change it to isOpeningMainEntryPoint
instead.
* and one for the desktop. Once these are merged, we can't hardcode the | ||
* platform here, and need to detect it from the environment. */ | ||
const currentPlatform = contentConfig.OPTIONS.groups.startup.options.platform.value | ||
const webPlatform = contentConfig.OPTIONS.groups.startup.options.platform.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.
This line is very tricky. It will stop working if we change the default there, which makes the code really hard to refactor. I think it would be better (still far from best solution known from real typed languages) to do if (currentPlatform == 'web')
. This will work even if we change defaults.
|
||
// @ts-expect-error `globalConfig.windowAppScopeName` is not known at typecheck time. | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
window[GLOBAL_CONFIG.windowAppScopeName] = API |
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.
What happened with this line? We are accessing the JS app from Rust via this variable. Why was it deleted? Was it moved somewhere? Such changes as this should be very clearly explained as they can break a lot of things.
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.
might be good to add a comment about this needing to be exposed to Rust
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.
@wdanilo are you sure that rust uses this global? this assignes Main to window.enso
which I can't find any references in code to be used.
App also expose ensoglApp
in window https://github.com/enso-org/enso/blob/develop/lib/rust/ensogl/pack/js/src/runner/index.ts#L217 which is widely accessed e.g. in gui/analytics/src/remote_log.js
. It also has a comment that rust access it.
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.
oh yeah - the initial commit (the revert commit) already had the global removed so all QAs seemed to work fine without the global.
i added it back because i figured it's safer to have it just in case
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.
Please, never remove lines that you are not 100% sure can be removed. If you want to refactor such a code, the PR description should have a very explicit information about it, so reviewers can be extra careful when checking this place. This removed line was used in Rust in several places, so this change was introducing regression.
project: projectManager.OpenProject | ||
} | ||
|
||
export class Backend implements Partial<Omit<cloudService.Backend, 'platform'>> { |
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.
No docs - there should be clear info of what a "backend" is, as this is not a name we are using in all other parts of the project.
this.currentlyOpenProject = null | ||
} | ||
|
||
async getProjectDetails(projectId: cloudService.ProjectId): Promise<cloudService.Project> { |
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.
If getPtojectDetails returns cloudService.Project
, it means that it can't get local projects info, right? This is surprising based on the naming Backend.getProjectDetails
- there is no info in these names that it works with cloud projects only. (The same comment applies to other similar functions).
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.
cloudService.Project
is just Project
, it's defined in cloudService
because it mirrors the return type from the cloud API. it's possible to rename the import (to something like types
or apiTypes
) to make this less confusing, if that's a satisfactory solution?
// === createBackend === | ||
// ===================== | ||
|
||
/** Shorthand method for creating a new instance of the backend API. */ |
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.
- Why
createBackend()
is "shorthand" fornew Backend()
, while the latter is in fact shorter? - The longer I read this file the more I believe the naming in this file is bad. Are we really creating new backend here? I think we are not spawning backend here.
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.
this file was derived from cloudService.ts
, but that's a good point. the "backend" in this case is the project manager, and it's creating a new api wrapper rather than a backend, but it's definitely more than doable to improve naming all around
/** Duration before the {@link ProjectManager} tries to create a WebSocket again. */ | ||
const RETRY_INTERVAL = 1000 | ||
/** Duration after which the {@link ProjectManager} stops re-trying to create a WebSocket. */ | ||
const STOP_TRYING_AFTER = 10000 |
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.
constants should either use typed numbers or encode unit in name. If these are milliseconds, these should be named RETRY_INTERVAL_MS
.
// ================= | ||
|
||
/** The address of the socket for the project manager. */ | ||
const PROJECT_MANAGER_ENDPOINT = 'ws://127.0.0.1:30535' |
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.
Is this a hardcoded thing? Is this agreed with backend? If so, docs should mention which other part of the project it should be synchronized with.
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.
should the other side also have a comment pointing to this file?
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.
CC @PabloBuchu - I was chatting with Pawel how to refactor it to a common file.
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.
It should be placed in app/gui/config.yaml
so both rust and js can access it
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.
should this be done in this PR?
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.
from our side imho yes. Let's put the const PROJECT_MANAGER_ENDPOINT = 'ws://127.0.0.1:30535'
to gui/config.yaml
and just use it in projectManager.ts
import GLOBAL_CONFIG from '../../../../gui/config.yaml' assert { type: 'yaml' }
...
return (this.instance ??= new ProjectManager(GLOBAL_CONFIG.projectManagerEndpoint))
@@ -189,6 +191,9 @@ export class App { | |||
mainEntryPoints = new Map<string, wasm.EntryPoint>() | |||
progressIndicator: wasm.ProgressIndicator | null = null | |||
initialized = false | |||
/** Field indicating that application was stopped and any long running process (like wasm compilation) should be aborted. |
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.
100 chars
lots of changes so will almost certainly need to be QA'd again |
@@ -0,0 +1,10 @@ | |||
/** @file Interfaces common to multiple modules. */ | |||
|
|||
interface StringConfig { |
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.
no sections
[key: string]: StringConfig | string | ||
} | ||
|
||
interface AppRunner { |
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.
no docs - there should be info why we need it and what it is used for.
|
||
// @ts-expect-error `globalConfig.windowAppScopeName` is not known at typecheck time. | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
window[GLOBAL_CONFIG.windowAppScopeName] = API |
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.
Please, never remove lines that you are not 100% sure can be removed. If you want to refactor such a code, the PR description should have a very explicit information about it, so reviewers can be extra careful when checking this place. This removed line was used in Rust in several places, so this change was introducing regression.
// This type annotation is explicit to undo TypeScript narrowing to `false`, | ||
// which result in errors about unused code. | ||
// eslint-disable-next-line @typescript-eslint/no-inferrable-types | ||
const EXPERIMENTAL: boolean = true | ||
const EXPERIMENTAL: boolean = false |
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.
The docs above do not tell the reader what features we are talking about. Thus, it is really hard to review this code, as I don't understand which features we disable. If you want to keep this with explicit info what it enables, you can do similar to what we do in Rust:
const EXPERIMENTAL = {
nameOfFeatureOne: true,
nameOfFeatureTwo: false,
}
import * as backend from '../service' | ||
import * as backendApi from '../cloudBackendApi' |
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.
- Why are we changing the name from
backend
tobackendApi
? Every file defines API, so in fact, every file could be named...Api
. - It should be imported as
cloudBackend
, as it's now confusing with normal Enso's backend.
/** @file Module containing the API client for the local backend API. | ||
* | ||
* Each exported function in the {@link ProjectManagerBackendAPI} in this module corresponds to an API endpoint. The | ||
* functions are asynchronous and return a `Promise` that resolves to the response from the API. */ |
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 was chatting with Pawel here. Withotu this chat I was totally confused what is this file about and what is the difference between "Project Manager Backend" and "Project Manager". This should be named probably "localBackend", so it will nicely fit the "cloudBackend" name.
Also, please double check if these docs are correct, as I think they refer improper files.
@@ -1,11 +1,12 @@ | |||
/** @file Module containing the API client for the Cloud backend API. | |||
* | |||
* Each exported function in the {@link Backend} in this module corresponds to an API endpoint. The | |||
* Each exported function in the {@link CloudBackendAPI} in this module corresponds to an API endpoint. The |
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.
Paweł's comment here - if we are changing one of the files to localBackend
, this one can be changed to remoteBackend
to keep it even more synced
const CHECK_STATUS_INTERVAL = 10000 | ||
/** The interval between requests checking whether the VM is ready. */ | ||
const CHECK_RESOURCES_INTERVAL = 5000 |
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.
- These values are huge. If I understand correctly, these are 10s between checks for IDE status?
- Names should have
_MS
suffix.
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.
- that is correct - i think that is what it was like on the old frontend
- will fix asap
re: 1 - it seems like the backend may get overloaded if these are sent too quickly. in particular i tested the CHECK_RESOURCES_INTERVAL and multiple were returning as successful because the old interval was a lot shorter than the response time
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.
also worth noting that instances currently still take ~90s to start up so i think the 10 second interval isn't such a big deal, at least for now
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.
lets check resources every 1second and status interval every 5 seconds
// ================= | ||
|
||
/** The address of the socket for the project manager. */ | ||
const PROJECT_MANAGER_ENDPOINT = 'ws://127.0.0.1:30535' |
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.
CC @PabloBuchu - I was chatting with Pawel how to refactor it to a common file.
latest commit introduces a |
this will definitely need another QA, not sure whether the reviews are more or less done though |
@somebody1234 is this ready for QA? |
@sylwiabr i think there are no further reviews for now so yeah, should be |
I am doing QA session on mac for this PR. Could @mwu-tow take a look and click through this on windows as well? Some useful tricks:
|
note re: the shortcut - it's mostly there for debugging and can be changed/removed, especially when #6474 is merged in (not that this will change QA - just noting this here for posterity) |
QA on mac looks 🟢 I created some minor improvements and bugs tickets but nothing blocking this PR. Lets do another QA session on different platform and merge |
Done testing discussed all the observed issues with @PabloBuchu who will be creating issues to track them. |
* develop: (34 commits) Continued Execution Context work and some little fixes (#6506) IDE's logging to a file (#6478) Fix application config (#6513) Cloud/desktop mode switcher (#6448) Fix doubled named arguments bug (#6422) Reimplement `enso_project` as a proper builtin (#6352) Fix layer ordering between dropdown and breadcrumbs backgrounds. (#6483) Multiflavor layers (#6477) DataflowAnalysis preserves dependencies order (#6493) Implement `create_database_table` for Database Table (#6467) Limit Dead Letter logging (#6482) More reliable shutdown of the EnsoContext to save resources (#6468) Make execution mode `live` default for CLI (#6496) Finishing Vector Editor (#6470) Proper handling of multiple list views. (#6461) Fix disappearing cached shapes (#6458) Add Execution Context control to Text.write (#6459) Change defaults for `Connection.tables` and ensure that `Connection.query` recognizes all available tables (#6443) Introducing @BuiltinMethod.needsFrame and InlineableNode (#6442) Hide profile button behind a feature flag (#6430) ...
* develop: Fix cut-off in text visualisations (#6421) Infer correct synthetic name for nested modules (#6525) Delete unused websocket dependency (#6535) Run typecheck and eslint on `./run lint` (#6314) Force pending saves if client closes abruptly (#6514) Continued Execution Context work and some little fixes (#6506) IDE's logging to a file (#6478) Fix application config (#6513) Cloud/desktop mode switcher (#6448) Fix doubled named arguments bug (#6422) Reimplement `enso_project` as a proper builtin (#6352) Fix layer ordering between dropdown and breadcrumbs backgrounds. (#6483) Multiflavor layers (#6477) DataflowAnalysis preserves dependencies order (#6493) Implement `create_database_table` for Database Table (#6467) Limit Dead Letter logging (#6482) More reliable shutdown of the EnsoContext to save resources (#6468) Make execution mode `live` default for CLI (#6496)
Pull Request Description
This is a re-creation of #6308.
Creates buttons to switch between cloud and local backends for listing directories, opening projects etc.
Important Notes
The desktop backend currently uses a hardcoded list of templates, mostly because they look better because they have background images. However, it can easily be changed to use
listSamples
endpoint and switched to the default grey background.Screenshots
The switcher is on the top left. UI is a placeholder and so it is subject to change.
![image](https://user-images.githubusercontent.com/4046547/234814131-b6964fe9-26d4-4749-8c33-2f014a049200.png)
![image](https://user-images.githubusercontent.com/4046547/234814193-f74ad0ec-0a29-4677-a423-85f4a4381e49.png)
Cloud backend:
Desktop backend:
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build
.