Move RemoteBackend and LocalBackend to enso-common for use in other modules
#14212
Conversation
somebody1234
added
the
CI: No changelog needed
| @@ -1,11 +1,11 @@ | |||
| import { Rfc3339DateTime } from 'enso-common/src/utilities/data/dateTime' | |||
| import { describe, expect, it, test } from 'vitest' | |||
| import { Rfc3339DateTime } from '../../utilities/data/dateTime.js' | |||
There was a problem hiding this comment.
Why did we start needing .js in imports?
There was a problem hiding this comment.
More importantly, why do we have .js files instead of .ts
There was a problem hiding this comment.
Why did we start needing
.jsin imports?
we don't, but it is (was?) good practice, since allowing .js to be ommitted is actually a feature specific to node.js (and node.js compatible runtimes)
More importantly, why do we have
.jsfiles instead of.ts
this is a quirk of typescript - it typically accepts .js as extensions, not .ts, as it does not rewrite import URLs (or at least did not until recently) - and the compiled output is typically expected to have .js extension.
There was a problem hiding this comment.
(let me know if i should remove the .js extensions anyway since most/all bundlers don't require the extension)
There was a problem hiding this comment.
I prefer without extensions
app/electron-client/src/index.ts
Outdated
| electron.app.quit() | ||
| }) | ||
| interface App { | ||
| window: import('electron').BrowserWindow | null |
There was a problem hiding this comment.
Why we use such way of importing here? Cannot we just put normal type import?
Also I don't qute get what was wrong with App being a class?
There was a problem hiding this comment.
what was wrong with App being a class
technically nothing, but i do think it makes more difficult to split functions more finely into separate files for maintainability
| export interface DownloadUrlOptions { | ||
| readonly url: string | ||
| readonly path?: Path | null | undefined | ||
| readonly name?: string | null | undefined | ||
| readonly shouldUnpackProject?: boolean | ||
| readonly showFileDialog?: boolean | ||
| } |
There was a problem hiding this comment.
I think this should be placed elsewhere. "Utilities" is too general, I don't see where I should use this structure (it mentions both FileDialog and project unpacking)
| /** Options for `download` function. */ | ||
| export interface DownloadOptions { | ||
| readonly url: string | ||
| readonly name?: string | null | undefined | ||
| readonly electronOptions?: Omit<DownloadUrlOptions, 'name' | 'url'> | ||
| } |
There was a problem hiding this comment.
If these are options for "download" function, it should be defined next to that function IMO
There was a problem hiding this comment.
true, but unfortunately right now the download function is browser-only :/ any ideas?
There was a problem hiding this comment.
Hmm, one is used in electron IPC, another in special download callback in Backends...
I think I would put this file just in src, like options.ts or text.ts
|
I don’t understand changes in UPDATE: Please also extend the PR description to include all changes made. It talks about moving stuff around to |
|
okay, makes sense. initial comments:
|
Pull Request Description
LocalBackend,RemoteBackend, and all dependencies toenso-common.Important Notes
None
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
The documentation has been updated, if necessary.Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,or the Snowflake database integration, a run of the Extra Tests has been scheduled.
If applicable, it is suggested to paste a link to a successful run of the Extra Tests.