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

Refactor npm package and add Worker support #203

Merged
merged 13 commits into from
Apr 26, 2023
Merged

Conversation

billti
Copy link
Member

@billti billti commented Apr 24, 2023

This PR cleans up the npm package so nearly all code is shared between node.js and the browser, with just a few lines in platform specific entry points. The platform specific entry points are now handled with 'conditional exports' in the package.json, so either Node.js or browser source consuming the package should just "import { ... } from 'qsharp'".

This PR also adds support for Worker threads, and moves the playground to use WebWorkers by default (getting all the wasm work such as compiling and simulating off the main UX thread). As this works similarly in both browser and Node.js, you can see in some of the unit tests added under ./npm/tests how this is used. (It's mostly the same as non-worker usage, just with one-line difference in initialization and clean-up.

As part of getting the above working, I also added configurable logging support. See ./npm/src/log.ts for details on that. (It should be pretty easy to wire this up to both VS Code "output channels" and macros from the Rust 'log' crate also).

I also made a lot of the TypeScript code more 'strongly typed', and it deals with a lot of string parsing and message handling in the 'npm' package now, avoid the need for consumers to do so.

Running the katas works, but I need to clean up the integration in the playground still for some of the refactoring before they are showing in the playground again (just commented out for now). I'll do that as part of a bigger playground UX clean-up.

I touched nearly every line of code in the npm package, so probably just easier to look at the code overall there, rather than the diff. Sorry :-/

npm/src/browser.ts Outdated Show resolved Hide resolved
npm/src/node-worker.ts Outdated Show resolved Hide resolved
npm/src/log.ts Outdated Show resolved Hide resolved
@swernli swernli mentioned this pull request Apr 25, 2023
@billti billti marked this pull request as ready for review April 26, 2023 08:15
Copy link
Collaborator

@swernli swernli left a comment

Choose a reason for hiding this comment

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

Signing off for the one change under compiler (addition of todo). Additionally, this PR could be a good place to move the compiler/qsc_wasm folder to a top-level wasm folder.

@billti billti enabled auto-merge (squash) April 26, 2023 20:45
@billti billti disabled auto-merge April 26, 2023 20:58
@billti billti merged commit 596e0fc into main Apr 26, 2023
10 checks passed
@billti billti deleted the billti/npm-refactor branch April 26, 2023 21:00
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.

None yet

3 participants