Skip to content

establish async dynamic import logic ✅ #542

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

Merged
merged 4 commits into from
Jul 28, 2025
Merged

Conversation

Freymaurer
Copy link
Collaborator

@Freymaurer Freymaurer commented Jul 27, 2025

The issue

When using ARCtrl in the browser with vite bundler you get error messages like such on bundling:

error during build:
node_modules/@nfdi4plants/arctrl/dist/ts/esm/ts/ContractIO/FileSystemHelper.js (3:15): "dirname" is not exported by "__vite-browser-external", imported by "node_modules/@nfdi4plants/arctrl/dist/ts/esm/ts/ContractIO/FileSystemHelper.js".
file: C:/Users/Kevin/Desktop/react-test/node_modules/@nfdi4plants/arctrl/dist/ts/esm/ts/ContractIO/FileSystemHelper.js:3:15

1: import { PromiseBuilder__Delay_62FBFDE1, PromiseBuilder__Run_212F1D4B } from "../fable_modules/Fable.Promise.3.2.0/Pr...
2: import { promise } from "../fable_modules/Fable.Promise.3.2.0/PromiseImpl.fs.js";
3: import { join, dirname } from "path";
                  ^
4: import { FsSpreadsheet_FsWorkbook__FsWorkbook_toXlsxFile_Static, FsSpreadsheet_FsWorkbook__FsWorkbook_fromXlsxFile_St...
5: import { substring, replace, trim as trim_1 } from "@fable-org/fable-library-js/String.js";

This error originates from the following code:

[<Erase>]
type PathModule =
    [<ImportMember("path")>]
    static member dirname (filePath : string) : string = jsNative

    [<ImportMember("path")>]
    static member join ([<ParamSeq>] paths : string []) : string = jsNative

This transpiles to a hard import in js. Which produces the error.

The solution

dynamic imports in ndoe environment:

export async function readConfigFile(path: string) {
  if (typeof process !== "undefined" && process.versions?.node) {
    // Only dynamically import in Node
    const { readFile } = await import("fs/promises");
    return await readFile(path, "utf-8");
  } else {
    throw new Error("readConfigFile is not available in the browser.");
  }
}

This syntax only imports the relevant function in node environment but produces a async function.

i established an fsharp emit, that is able to reproduce this syntax interopping with f# function syntax.

Because dynamic imports must be done in an async function i cannot use this syntax for the path functions above as they are not async.

@HLWeil can we make these functions promises or find another workaround? Would love to get your input on this before making system wide changes.

We could:

TODO

  • add a test react project which uses local build output for arctrl and try building it with CI.

    -> will do this as soon as it builds once successfully

  • streamline bundle process using vite lib mode

@Freymaurer Freymaurer requested a review from HLWeil July 27, 2025 10:02
@HLWeil
Copy link
Member

HLWeil commented Jul 27, 2025

Thanks for getting to this @Freymaurer!
I think it would be no problem to make these functions return promises, as they are mostly (if not completely) used in a promise context anyways.

@HLWeil
Copy link
Member

HLWeil commented Jul 27, 2025

Alternatively, after looking into this code, I guess replacing these functions might be the easier solution. We do have a join function in the Filesystem project: ARCtrl.ArcPathHelper.combine

dirname should also be straightforward to replace.

@Freymaurer
Copy link
Collaborator Author

Thanks for getting to this @Freymaurer! I think it would be no problem to make these functions return promises, as they are mostly (if not completely) used in a promise context anyways.

I just checked and they are actually only used inside the same file and only in async functions. i will make them async, as i think this is the easiest approach for me

Copy link
Member

@HLWeil HLWeil left a comment

Choose a reason for hiding this comment

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

Nice work!

@Freymaurer Freymaurer merged commit 0a3037c into main Jul 28, 2025
2 checks passed
@omaus
Copy link
Collaborator

omaus commented Jul 28, 2025

"Unexist typo" 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants