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

[Wasm] Add support for baseUrl and mainScriptPath #70375

Closed
wants to merge 1 commit into from

Conversation

jeromelaban
Copy link
Contributor

This change allows for resources like the crypto worker or thread workers to be resolved properly when used in a generic context.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 7, 2022
@jeromelaban jeromelaban changed the title feat: Add support for baseUrl and mainScriptPath [Wasm] Add support for baseUrl and mainScriptPath Jun 7, 2022
@pavelsavara
Copy link
Member

See also this #69441

} else {
module.mainScriptPath = hr;
}

(<any>module)["mainScriptUrlOrBlob"] = hr;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

while we're changing this, it might make sense to have a convention for how to tell the runtime what its baseUrl is so it doesn't have to guess. i'm guessing uno could pipe that through

Copy link
Member

@lambdageek lambdageek Jun 8, 2022

Choose a reason for hiding this comment

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

The guessing should be removed - and blazor updated to provide a base URL (and possibly also the URL of the worker script - actually depending on how much stuff they mangle we might need some locate function where we can ask for normal names that we know and blazor can tell us what they mangled them to). the guessing was just a hack because I didn't want to coordinate a pair of aspnetcore and runtime PRs

(I don't mean this current PR should remove the guessing and wait until blazor is updated - that should be done separately)

This change allows for resources like the crypto worker or thread workers to be resolved properly when used in a generic context.
@jeromelaban jeromelaban marked this pull request as ready for review June 8, 2022 17:00
@pavelsavara
Copy link
Member

guys, I don't have bandwidth to fully explore this space right now, but please make sure that you align it with NodeJS and that we have some consistent plan for the relative paths of configured resources. We have open discussion on the PR I mentioned above and also we have @radical 's host and maybe some new shape of the config.

All that said, I think we should just add dotnet-crypto-worker.js into mono-config.json as new resource type, rather than hard-code it here.

@pavelsavara
Copy link
Member

Crypto worker is gone now.
For threads worker we do resolve_asset_path which uses now improved locateFile.

const asset = resolve_asset_path("js-module-threads");
const uri = asset.resolvedUrl;
mono_assert(uri !== undefined, "could not resolve the uri for the js-module-threads asset");
const worker = new Worker(uri);

So, I guess this PR is obsolete ? @jeromelaban

@jeromelaban
Copy link
Contributor Author

@pavelsavara I guess it is! Thanks for the update.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices.JavaScript community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants