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

feat!: Remove js-runner #1585

Merged
merged 21 commits into from
Apr 23, 2023
Merged

feat!: Remove js-runner #1585

merged 21 commits into from
Apr 23, 2023

Conversation

ospencer
Copy link
Member

@ospencer ospencer commented Jan 9, 2023

Closes #645
Closes #1537

This PR removes the old js-runner. The recommended way to run Grain programs is via standard wasi-supporting tools!

Key points: -p is no longer supported by the CLI, and --no-link can only be used with grain compile (the cli no longer supports running unlinked programs).

Tested and confirmed working with pkg.

cli/bin/grain.js Outdated Show resolved Hide resolved
Copy link
Member

@peblair peblair left a comment

Choose a reason for hiding this comment

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

🪦

Copy link
Member

@marcusroberts marcusroberts left a comment

Choose a reason for hiding this comment

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

The times they are a-changing!

@ospencer
Copy link
Member Author

Thanks to some investigation by @phated, we learned the two reasons the Windows tests are failing are because the process clock has only millisecond resolution (which is too imprecise for this test to pass) and the thread clock doesn't work. I sent nodejs/uvwasi#181 and nodejs/uvwasi#182 to address these. Hopefully they make it into Node 19, and we'll be able to ship this 😎

@spotandjake
Copy link
Member

Given this will drop support for running in unlinked mode issue: #753 can probably be closed whenever this gets merged.

@phated phated requested a review from jozanza as a code owner April 18, 2023 05:21
cli/bin/grain.js Outdated Show resolved Hide resolved
cli/bin/exec.js Outdated
Comment on lines 131 to 132
const doubleDash = process.argv.indexOf("--");
const args = process.argv.slice(doubleDash + 1);
Copy link
Member

Choose a reason for hiding this comment

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

@ospencer Thoughts on doing this to pass the trailing argv to the WASI runner?

cli/bin/exec.js Outdated Show resolved Hide resolved
@phated
Copy link
Member

phated commented Apr 19, 2023

One question I've had since last night: How do we want to support env vars via the CLI? I think we can probably do the same thing as with preopens: --env=FOO=BAR. WDYT?

@ospencer
Copy link
Member Author

That was my thinking exactly.

@phated phated enabled auto-merge April 23, 2023 01:19
@phated phated added this pull request to the merge queue Apr 23, 2023
Merged via the queue into main with commit e10d612 Apr 23, 2023
@phated phated deleted the oscar/remove-js-runner branch April 23, 2023 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

GrainRunner: Use the real module name over the url JS Runtime: Expose grainToJSVal
5 participants