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.

Sign up for GitHub

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

Jump to bottom

Fix showLogs #7568

Closed
wants to merge 4 commits into from
Closed

Fix showLogs #7568

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5c6c752
Attempt to fix `showLogs()` after opening multiple cloud projects
somebody1234 Jul 5, 2023
9fe280f
Merge branch 'develop' into wip/sb/fix-showlogs
PabloBuchu Jul 18, 2023
fe135ac
Merge branch 'develop' into wip/sb/fix-showlogs
PabloBuchu Jul 21, 2023
d01cf2a
Merge branch 'develop' into wip/sb/fix-showlogs
somebody1234 Aug 11, 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
24 changes: 16 additions & 8 deletions lib/rust/ensogl/pack/js/src/runner/log/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ const consoleLogNames = [
'groupEnd',
] satisfies (keyof Console)[]

// If `showLogs` exists, then another `Router` instance has already been applied to `console`,
// and a new `Router` SHOULD NOT be created to wrap around the other `Router`.
const shouldExecute = !('showLogs' in host.global)

// FIXME: fix Rust `autoFlush` handling
/** Router for logs. It is used to hide the incoming logs and show them on demand. It is used to
* unclutter the logs view when the app is run by end-user. */
Expand All @@ -30,10 +34,12 @@ class Router {
this.buffer = []
this.console = {}
this.autoFlush = true
for (const name of consoleLogNames) {
this.console[name] = console[name]
console[name] = (...args: unknown[]) => {
this.consume(name, args)
if (shouldExecute) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look right to me that we need such guards. If this code is being executed for the second time, we are already creating an extra new Router() instance anyway, which I believe is incorrect. Usually top level code in JS modules have a guarantee of executing at most once, so it would be better to figure out why this rule is violated and fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Frizi the issue is that (currently) the frontend loads the application from the cloud, because the cloud may be running a different version of enso.runApp...?

i don't remember the reasoning why i made the editor do that tbh. i can create an alternate PR that fixes this a different way

for (const name of consoleLogNames) {
this.console[name] = console[name]
console[name] = (...args: unknown[]) => {
this.consume(name, args)
}
}
}
}
Expand Down Expand Up @@ -86,7 +92,9 @@ class Router {

export const router = new Router()

host.exportGlobal({
hideLogs: router.hideLogs.bind(router),
showLogs: router.showLogs.bind(router),
})
if (shouldExecute) {
host.exportGlobal({
hideLogs: router.hideLogs.bind(router),
showLogs: router.showLogs.bind(router),
})
}
Loading