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

Docs & Example for "Roll Your Own JS Runtime" blog posts #824

Merged
merged 7 commits into from
Sep 6, 2024

Conversation

NfNitLoop
Copy link
Contributor

I was surprised to find that one of the blog posts provided docs for the CreateSnapshotOptions instead of just linking to its Rustdoc. That's because the type itself didn't have docs.

The first commit here is me just inlining those docs into Rustdocs.

But then, as I tried following the example, I found that the APIs have diverged significantly from the blog post series. So I've added another commit which adds an example project that can serve as a live/working example of creating a snapshot for JsRuntime. Hopefully, since this will get built with cargo build, it'll stay more up-to-date than the blog posts.

If this goes too far, I'm happy to create a separate PR that is just the first commit.

If you'd like me to go further, I think it might be a good idea to split the extension out into a separate ext crate, which I think allows for code re-use between build.rs and main.rs.

commit eb35c83

Add docs for create_snapshot() and its options.

commit 618d423

Add an example for "Roll Your Own JS Runtime"

The blog post series had become out-of-date.

Committing example code will hopefully bit-rot less?

@CLAassistant
Copy link

CLAassistant commented Jul 13, 2024

CLA assistant check
All committers have signed the CLA.

@@ -4,6 +4,7 @@
resolver = "2"
members = [
"core",
"core/examples/snapshot",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the only way I found to get this example crate to run build.rs, which is required for generating the V8 Snapshot. But I'd be happy to learn that there's a better way.

The blog post series had become out-of-date.

Committing example code will hopefully bit-rot less?
@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.90%. Comparing base (0c7f83e) to head (95f1036).
Report is 115 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #824      +/-   ##
==========================================
+ Coverage   81.43%   81.90%   +0.46%     
==========================================
  Files          97       98       +1     
  Lines       23877    24929    +1052     
==========================================
+ Hits        19445    20417     +972     
- Misses       4432     4512      +80     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iuioiua
Copy link
Contributor

iuioiua commented Jul 20, 2024

@bartlomieju, PTAL

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Looks cool, can you update .github/workflows/ci.yml CI script to actually run core/examples/snapshot to ensure it's working fine and prints correct output? Let me know if you need any help or pointers.

core/runtime/snapshot.rs Outdated Show resolved Hide resolved
core/runtime/snapshot.rs Outdated Show resolved Hide resolved
@NfNitLoop
Copy link
Contributor Author

@bartlomieju Done. No need to update the workflow, it already runs cargo test for the workspace, so I just dropped a little self-contained integration test into the example dir.

Result:
https://github.com/denoland/deno_core/actions/runs/10064433085/job/27821343522?pr=824#step:13:43

@irbull
Copy link
Contributor

irbull commented Aug 7, 2024

+1. I just took this for a spin, and from a user perspective this works really well.

Only comment, maybe mention in the README.md in examples/snapshot/ dir that you can simply run

cargo run

To try the example. Maybe that's obvious, but as it was in the examples directory I first thought we could use cargo run --example snapshot, but it's actually a binary crate.

Great work!

@irbull
Copy link
Contributor

irbull commented Aug 7, 2024

One more note, Deno commits typically follow Conventional Commits. I think these commits can be squashed when merged. Here is a proposed commit message from the 5 commits in this change-set.

docs: add "Roll Your Own JS Runtime" Example

Creates an example that demonstrates how to roll your own JS runtime using Deno Core.
In particular, this example shows how to use a build script (`build.rs`) to create a snapshot,
and how to load the snapshot when the JS runtime is started.

This change-set also includes documentation for `create_snapshot()` and its options.

@irbull
Copy link
Contributor

irbull commented Aug 20, 2024

Anything I can do to help push this one through? I can rebase this and update the commit message if that helps? I can add my suggestion about the docs for cargo run. Let me know if I can help in any way.

@NfNitLoop
Copy link
Contributor Author

Likewise. If there are any blockers to merging this, I'm happy to make them.

@devsnek devsnek self-requested a review September 5, 2024 16:36
Co-authored-by: Ian Bull <irbull@gmail.com>
@devsnek devsnek enabled auto-merge (squash) September 5, 2024 17:45
@irbull
Copy link
Contributor

irbull commented Sep 5, 2024

The build error here is related to a change in Rust nightly. Specifically, the linter changed when elided lifetimes end up becoming named ones. I've put together a PR to fix this: #895

@irbull
Copy link
Contributor

irbull commented Sep 6, 2024

The build error here is related to a change in Rust nightly. Specifically, the linter changed when elided lifetimes end up becoming named ones. I've put together a PR to fix this: #895

The PR that fixes the build failure is now merged. I can't seem to rebase / merge main into this PR so someone else will need to do that. Hopefully then we can merge this.

@devsnek devsnek merged commit f4fad69 into denoland:main Sep 6, 2024
18 checks passed
kuruk-mm pushed a commit to dclexplorer/deno_core that referenced this pull request Oct 11, 2024
I was surprised to find that one of the blog posts [provided docs][1]
for the `CreateSnapshotOptions` instead of just linking to its Rustdoc.
That's because the type itself [didn't have docs][2].

The first commit here is me just inlining those docs into Rustdocs.

But then, as I tried following the example, I found that the APIs have
diverged significantly from the blog post series. So I've added another
commit which adds an example project that can serve as a live/working
example of creating a snapshot for JsRuntime. Hopefully, since this will
get built with `cargo build`, it'll stay more up-to-date than the blog
posts.

If this goes too far, I'm happy to create a separate PR that is just the
first commit.

If you'd like me to go further, I think it might be a good idea to split
the extension out into a separate `ext` crate, which I think allows for
code re-use between `build.rs` and `main.rs`.

[1]:
https://deno.com/blog/roll-your-own-javascript-runtime-pt3#diving-into-createsnapshotoptions
[2]:
https://docs.rs/deno_core/0.294.0/deno_core/snapshot/struct.CreateSnapshotOptions.html


commit eb35c83

    Add docs for create_snapshot() and its options.

commit 618d423

    Add an example for "Roll Your Own JS Runtime"
    
    The blog post series had become out-of-date.
    
    Committing example code will hopefully bit-rot less?

---------

Co-authored-by: snek <the@snek.dev>
Co-authored-by: Ian Bull <irbull@gmail.com>
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.

7 participants