Skip to content

Add virtual filesystem support via executeWithFs#12

Closed
aybanda wants to merge 3 commits intotscircuit:mainfrom
aybanda:feature/virtual-filesystem
Closed

Add virtual filesystem support via executeWithFs#12
aybanda wants to merge 3 commits intotscircuit:mainfrom
aybanda:feature/virtual-filesystem

Conversation

@aybanda
Copy link
Copy Markdown

@aybanda aybanda commented Dec 10, 2024

Adds .executeWithFs() method to support virtual filesystem and relative imports.

Changes

  • Added executeWithFs method to API
  • Implemented virtual filesystem in worker
  • Added type definitions
  • Added test coverage

Testing

Screenshot 2024-12-10 at 2 44 38 PM

/claim #11

Copy link
Copy Markdown
Contributor

@seveibar seveibar left a comment

Choose a reason for hiding this comment

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

this looks great, but you gotta install biome and format because your diff is huge and I can't tell what you changed- mostly because of semicolons being added everywhere


const circuitJson = await circuitWebWorker.getCircuitJson()
const circuitJson = await circuitWebWorker.getCircuitJson();
```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use git checkout origin/main -- README.md on files you didn't mean to change

check your work by looking at files changed and make sure you're not submitting unnecessary code

const proxiedCallback = Comlink.proxy(callback)
webWorker.on(event, proxiedCallback)
},
async executeWithFs({ fsMap, entrypoint }: ExecuteWithFsOptions) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

look at the syntax for the other files- why change the syntax?

interface ExecuteWithFsOptions {
fsMap: Record<string, string>
entrypoint: string
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you've defined this type twice

if (!fsMap[fullPath]) {
throw new Error(`Import not found in filesystem: ${fullPath}`)
}
return `// Virtual import: ${fullPath}\n${fsMap[fullPath]}`
Copy link
Copy Markdown
Contributor

@seveibar seveibar Dec 10, 2024

Choose a reason for hiding this comment

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

this technique...

  1. doesn't work for json files or other extensions
  2. changes the exports of the main module, will cause conflicting duplicate exports
  3. doesn't handle recursive imports or modules that import other modules

@seveibar
Copy link
Copy Markdown
Contributor

This was a good attempt but it's taking more time to review this than to write it

/tip $5

@seveibar seveibar closed this Dec 10, 2024
@algora-pbc
Copy link
Copy Markdown

algora-pbc bot commented Dec 10, 2024

🎉🎈 @aybanda has been awarded $5! 🎈🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants