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

fix(api-base): resolve resource promises in parallel #3

Merged
merged 3 commits into from
Jan 6, 2024

Conversation

thoughtpolice
Copy link
Contributor

Summary: When fetching built-in resources (e.g. techlibs) for the virtual filesystem, do it in parallel by building up an array of promises and resolving them via Promise.all, rather than doing so sequentially.

A nice effect of this is that it helps avoid HOL blocking when the resource might be non-local e.g. your in-browser HTTP request to a CDN or server suddenly starts having bad latency.

It also lets us get away with less logging (ref: #2), and removing these explicit console.log calls actually fixes an awkward case where printLine is set to a no-op function to keep things quiet, but these logs would still get output with no way to suppress it. (If this is ever brought back it should at least respect the printLine callback.)

Fixes #2.

Change-Id: Ie8085a216c22576d6956f007e2859e2381a56a12

@thoughtpolice
Copy link
Contributor Author

I ran the testsuite, so I hope this is OK, but please keep my baby JavaScript skills in mind.

Summary: When fetching built-in resources (e.g. techlibs) for the virtual
filesystem, do it in parallel by building up an array of promises and resolving
them via `Promise.all`, rather than doing so sequentially.

A nice effect of this is that it helps avoid HOL blocking when the resource
might be non-local e.g. your in-browser HTTP request to a CDN or server suddenly
starts having bad latency.

It also lets us get away with less logging (ref: YoWASP#2), and removing these
explicit `console.log` calls actually fixes an awkward case where `printLine`
is set to a no-op function to keep things quiet, but these logs would still get
output with no way to suppress it. (If this is ever brought back it should at
least respect the `printLine` callback.)

Fixes YoWASP#2.

Signed-off-by: Austin Seipp <aseipp@pobox.com>
Change-Id: Ie8085a216c22576d6956f007e2859e2381a56a12
@thoughtpolice thoughtpolice force-pushed the aseipp/push-xuppoltqzosl branch from 0434b13 to 3d047fd Compare January 6, 2024 01:35
lib/api-base.js Outdated Show resolved Hide resolved
@whitequark
Copy link
Member

(If this is ever brought back it should at least respect the printLine callback.)

The package is primarily targeting the browser use case, where console.log is invisible. I agree that expanding its scope to Node/Deno is reasonable, and dropping these calls to console.log is also reasonable. I see no need to forward this information to printLine; that exists solely for the application itself, not for any debug information, since the embedder may want to interpret that info.

@whitequark
Copy link
Member

Note that once this is merged, all packages will need to (a) have the version bumped in package.json and (b) be re-published, since they bundle the runtime.

@whitequark whitequark merged commit 9e8cb7a into YoWASP:develop Jan 6, 2024
4 checks passed
@whitequark
Copy link
Member

Thanks!

@thoughtpolice
Copy link
Contributor Author

Thanks for the quick response! Much appreciated.

@thoughtpolice thoughtpolice deleted the aseipp/push-xuppoltqzosl branch January 6, 2024 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

console.log is a little verbose when using deno
2 participants