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

Cloud/desktop mode switcher #6448

Merged
merged 31 commits into from
May 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
3cf7185
Revert "Revert "Cloud/desktop mode switcher (#6308)" (#6444)"
somebody1234 Apr 27, 2023
810615d
Lazily initialize project manager
somebody1234 Apr 27, 2023
1970caf
Minor bugfix
somebody1234 Apr 27, 2023
4806ff3
Convert backends to a discriminated union; don't remove CLI args for …
somebody1234 Apr 27, 2023
1fce405
Rename `handleSubmit` to `onSubmit` for consistency
somebody1234 Apr 27, 2023
df4487e
Remove duplicate UI; disable experimental UI
somebody1234 Apr 27, 2023
efa3e1a
Address review
somebody1234 Apr 27, 2023
d0ddd31
Address review
somebody1234 Apr 27, 2023
857a36c
Move `new app.App` into `main`
somebody1234 Apr 27, 2023
a967203
Fixes
somebody1234 Apr 27, 2023
2a1ee22
Delay setting project state to `Opened` until backend is fully ready
somebody1234 Apr 25, 2023
b412c46
Retry getting project resources info in a loop
somebody1234 Apr 26, 2023
aaad9e0
Increase interval between polling `project/resources`
somebody1234 Apr 27, 2023
eb53552
Immediately set state to finished on desktop
somebody1234 Apr 27, 2023
b1653ef
Fixes; minor refactors; set project to ready when `openProject()` fin…
somebody1234 Apr 27, 2023
56a2579
Refactor `runApp` and `tryStopApp` into a class; avoid setting globals
somebody1234 Apr 28, 2023
5c2b112
Add `wasmUrl` and `assetsUrl` overrides back
somebody1234 Apr 28, 2023
25d0a1c
Style and markup fixes
somebody1234 Apr 28, 2023
a2b8c16
Remove placeholder
somebody1234 Apr 28, 2023
29e336b
Use `config.loadAll` to load command line options
somebody1234 Apr 28, 2023
889e9db
Fix creating blank project on desktop
somebody1234 Apr 28, 2023
db4c776
live review fixes
PabloBuchu Apr 28, 2023
4bc378d
Address review
somebody1234 Apr 28, 2023
522562c
Address review
somebody1234 May 1, 2023
e0fd0b6
Adjust backend check intervals
somebody1234 May 1, 2023
bb9730b
Minor bugfix
somebody1234 May 1, 2023
09ca400
Fix type error
somebody1234 May 1, 2023
4c56e7d
Extract project manager URL into `config.yaml`
somebody1234 May 1, 2023
5e1a29d
Fix dashboard watch
somebody1234 May 2, 2023
db2aecc
Finish renaming `cloudBackend` to `remoteBackend`
somebody1234 May 2, 2023
bb6530f
Merge branch 'develop' into wip/sb/cloud-desktop-mode-switcher
PabloBuchu May 2, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/gui/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ windowAppScopeConfigName: "config"
# utilities and allowing for runtime theme modification.
windowAppScopeThemeName: "theme"

# The URL to the JSON-RPC endpoint to the Project Manager.
# This MUST be kept in sync with the corresponding value in `app/gui/src/constants.rs`.
projectManagerEndpoint: "ws://127.0.0.1:30535"

# TODO [ao] add description here.
minimumSupportedVersion": "2.0.0-alpha.6"

Expand Down
4 changes: 4 additions & 0 deletions app/ide-desktop/eslint.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,10 @@ const RESTRICTED_SYNTAXES = [
'TSAsExpression:has(TSUnknownKeyword, TSNeverKeyword, TSAnyKeyword) > TSAsExpression',
message: 'Use type assertions to specific types instead of `unknown`, `any` or `never`',
},
{
selector: 'IfStatement > ExpressionStatement',
message: 'Wrap `if` branches in `{}`',
},
]

/* eslint-disable @typescript-eslint/naming-convention */
Expand Down
8 changes: 2 additions & 6 deletions app/ide-desktop/lib/content/esbuild-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,9 @@ export function bundlerOptions(args: Arguments) {
outbase: 'src',
plugins: [
{
// This is a workaround that is needed
// because esbuild panics when using `loader: { '.js': 'copy' }`.
// See https://github.com/evanw/esbuild/issues/3041.
// Setting `loader: 'copy'` prevents this file from being converted to ESM
// because of the `"type": "module"` in the `package.json`.
// This file MUST be in CommonJS format because it is loaded using `Function()`
// in `ensogl/pack/js/src/runner/index.ts`
// in `ensogl/pack/js/src/runner/index.ts`.
// All other files are ESM because of `"type": "module"` in `package.json`.
name: 'pkg-js-is-cjs',
setup: build => {
build.onLoad({ filter: /[/\\]pkg.js$/ }, async ({ path }) => ({
Expand Down
2 changes: 1 addition & 1 deletion app/ide-desktop/lib/content/src/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@
user-scalable = no"
/>
<title>Enso</title>
<link rel="stylesheet" href="/tailwind.css" />
<link rel="stylesheet" href="/style.css" />
<link rel="stylesheet" href="/docsStyle.css" />
<link rel="stylesheet" href="/tailwind.css" />
<script type="module" src="/index.js" defer></script>
<script type="module" src="/run.js" defer></script>
</head>
Expand Down
135 changes: 77 additions & 58 deletions app/ide-desktop/lib/content/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import * as authentication from 'enso-authentication'
import * as contentConfig from 'enso-content-config'

import * as app from '../../../../../target/ensogl-pack/linked-dist/index'
import * as projectManager from './project_manager'
import GLOBAL_CONFIG from '../../../../gui/config.yaml' assert { type: 'yaml' }

const logger = app.log.logger
Expand Down Expand Up @@ -119,15 +118,26 @@ function displayDeprecatedVersionDialog() {
}

// ========================
// === Main Entry Point ===
// === Main entry point ===
// ========================

interface StringConfig {
[key: string]: StringConfig | string
}

class Main {
async main(inputConfig: StringConfig) {
class Main implements AppRunner {
app: app.App | null = null

stopApp() {
this.app?.stop()
}

async runApp(inputConfig?: StringConfig) {
this.stopApp()

/** FIXME: https://github.com/enso-org/enso/issues/6475
* Default values names are out of sync with values used in code.
* Rather than setting fixed values here we need to fix default values in config. */
const config = Object.assign(
{
loader: {
Expand All @@ -139,7 +149,7 @@ class Main {
inputConfig
)

const appInstance = new app.App({
this.app = new app.App({
config,
configOptions: contentConfig.OPTIONS,
packageInfo: {
Expand All @@ -148,75 +158,84 @@ class Main {
},
})

if (appInstance.initialized) {
if (!this.app.initialized) {
console.error('Failed to initialize the application.')
} else {
if (contentConfig.OPTIONS.options.dataCollection.value) {
// TODO: Add remote-logging here.
}
if (!(await checkMinSupportedVersion(contentConfig.OPTIONS))) {
displayDeprecatedVersionDialog()
} else {
if (
(contentConfig.OPTIONS.options.authentication.value ||
contentConfig.OPTIONS.groups.featurePreview.options.newDashboard.value) &&
contentConfig.OPTIONS.groups.startup.options.entry.value ===
contentConfig.OPTIONS.groups.startup.options.entry.default
) {
const hideAuth = () => {
const auth = document.getElementById('dashboard')
const ide = document.getElementById('root')
if (auth) auth.style.display = 'none'
if (ide) ide.style.display = ''
}
/** This package is an Electron desktop app (i.e., not in the Cloud), so
* we're running on the desktop. */
/** TODO [NP]: https://github.com/enso-org/cloud-v2/issues/345
* `content` and `dashboard` packages **MUST BE MERGED INTO ONE**. The IDE
* should only have one entry point. Right now, we have two. One for the cloud
* 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 platform = authentication.Platform.desktop
/** FIXME [PB]: https://github.com/enso-org/cloud-v2/issues/366
* React hooks rerender themselves multiple times. It is resulting in multiple
* Enso main scene being initialized. As a temporary workaround we check whether
* appInstance was already ran. Target solution should move running appInstance
* where it will be called only once. */
let appInstanceRan = false
const onAuthenticated = () => {
if (
!contentConfig.OPTIONS.groups.featurePreview.options.newDashboard.value
) {
hideAuth()
if (!appInstanceRan) {
appInstanceRan = true
void appInstance.run()
}
}
}
authentication.run({
logger,
platform,
projectManager: projectManager.ProjectManager.default(),
showDashboard:
contentConfig.OPTIONS.groups.featurePreview.options.newDashboard.value,
onAuthenticated,
})
} else {
void appInstance.run()
}
const email = contentConfig.OPTIONS.groups.authentication.options.email.value
// The default value is `""`, so a truthiness check is most appropriate here.
if (email) {
logger.log(`User identified as '${email}'.`)
}
void this.app.run()
}
}
}

main(inputConfig?: StringConfig) {
contentConfig.OPTIONS.loadAll([app.urlParams()])
Copy link
Member

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 isUsingAuthentication = contentConfig.OPTIONS.options.authentication.value
const isUsingNewDashboard =
contentConfig.OPTIONS.groups.featurePreview.options.newDashboard.value
const isOpeningMainEntryPoint =
contentConfig.OPTIONS.groups.startup.options.entry.value ===
contentConfig.OPTIONS.groups.startup.options.entry.default
if ((isUsingAuthentication || isUsingNewDashboard) && isOpeningMainEntryPoint) {
const hideAuth = () => {
const auth = document.getElementById('dashboard')
const ide = document.getElementById('root')
if (auth) {
auth.style.display = 'none'
}
if (ide) {
ide.hidden = false
}
}
/** This package is an Electron desktop app (i.e., not in the Cloud), so
* we're running on the desktop. */
/** TODO [NP]: https://github.com/enso-org/cloud-v2/issues/345
* `content` and `dashboard` packages **MUST BE MERGED INTO ONE**. The IDE
* should only have one entry point. Right now, we have two. One for the cloud
* and one for the desktop. */
const currentPlatform = contentConfig.OPTIONS.groups.startup.options.platform.value
let platform = authentication.Platform.desktop
if (currentPlatform === 'web') {
platform = authentication.Platform.cloud
}
/** FIXME [PB]: https://github.com/enso-org/cloud-v2/issues/366
* React hooks rerender themselves multiple times. It is resulting in multiple
* Enso main scene being initialized. As a temporary workaround we check whether
* appInstance was already ran. Target solution should move running appInstance
* where it will be called only once. */
let appInstanceRan = false
const onAuthenticated = () => {
if (!contentConfig.OPTIONS.groups.featurePreview.options.newDashboard.value) {
hideAuth()
if (!appInstanceRan) {
appInstanceRan = true
void this.runApp(inputConfig)
}
}
}
authentication.run({
appRunner: this,
logger,
platform,
showDashboard:
contentConfig.OPTIONS.groups.featurePreview.options.newDashboard.value,
onAuthenticated,
})
} else {
console.error('Failed to initialize the application.')
void this.runApp(inputConfig)
}
}
}

const API = new Main()

// @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
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

@somebody1234 somebody1234 May 1, 2023

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

Copy link
Member

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.

window[GLOBAL_CONFIG.windowAppScopeName] = new Main()
39 changes: 0 additions & 39 deletions app/ide-desktop/lib/content/src/newtype.ts

This file was deleted.

Loading