Skip to content

Conversation

@coolreader18
Copy link
Collaborator

@coolreader18 coolreader18 commented Nov 5, 2025

Description of Changes

Currently need changes from both #3548 and #3527, and the latter is merged into master but the former isn't rebased on top of it. so we stay silly for the moment

I'd like to pull out my first commit into its own PR, but that's not really possible until #3548 rebases onto master.

Expected complexity level and risk

2 - pretty straightforward translation of the wasm/Rust view implementation to typescript.

Testing

@coolreader18 coolreader18 force-pushed the noa/ts-views branch 2 times, most recently from bf81d07 to 61c21e3 Compare November 5, 2025 21:29
@joshua-spacetime joshua-spacetime linked an issue Nov 6, 2025 that may be closed by this pull request
@coolreader18 coolreader18 force-pushed the noa/ts-views branch 2 times, most recently from ab7e5d0 to 9d0e017 Compare November 6, 2025 19:00
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

This generally looks good to me. I took a closer look at the TypeScript side and it seems fine to me, indeed I've made some of the same changes in #3559.

I would like to do additional testing though. I think we should at least add views and all the options and stuff to modules/module-test-ts

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

The host side looks good. I just have some minor suggestions.

Copy link
Collaborator

@joshua-spacetime joshua-spacetime left a comment

Choose a reason for hiding this comment

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

@coolreader18 Could you add this view to module-test-ts for the ensure_same_schema tests.

@joshua-spacetime
Copy link
Collaborator

@coolreader18 Could you also disable passing parameters? We can re-enable once we have sql support for it.

@coolreader18 coolreader18 added this pull request to the merge queue Nov 7, 2025
Merged via the queue into master with commit cc73953 Nov 7, 2025
24 of 25 checks passed
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.

Invoke V8 views from host

5 participants