Add support for forced-included or forced-loaded scripts to `extension!`

[mmastrac]

There are two major remaining uses of the extension customizer/extension builder in deno, and these are to specify scripts that should be included in the binary, or loaded from the FS during snapshot. We should replace these with an extension! DSL to specify them and remove the customizers.

When we do this, we should also be able to remove the remaining non-extension! implementations in the deno_core tests.

[nayeemrmn]

Just like with_specifier we can add a per-line with_code which overrides ExtensionFileSource::code.

[nayeemrmn]

Just like with_specifier we can add a per-line with_code which overrides ExtensionFileSource::code.

I guess that removes a lot of the benefit of the macro for something that could be an option.

I implemented something like force-included in https://github.com/denoland/deno/pull/19463. I wish there was a better way to coalesce these boolean arguments.

[mmastrac]

Do you have an idea of a syntax you'd like to see for this?

Bikeshedding a bit, maybe we can allow for object-literal-style scripts in the esm list::

esm = [ 
"actual_file.js", 
{ name: "main.js", specifier: "...", code: r#"
function test() {
  /* some embedded code */
}
"# } ]

[nayeemrmn]

Notes

• It's clear that dir "foo" should be a top-level field so we have parsing options for other stuff. esm = [] and js = [] can share it.
• code needs to accept ExtensionFileSourceCode rather than a string literal. It's worth it looking at CLI uses.
• TOML objects would probably fit more than JS-style.

So:

extension!(
  deno_node,
  esm_entry_point = "ext:deno_node/02_init.js",
  source_root_path = "polyfills",
  esm = [
    "02_init.js",
    { path = "assert.ts", specifier = "node:assert" }
    { specifier = "node:buffer", code = ExtensionFileSourceCode { ... } }
  ]
);

Variations other than { path, specifier }, { specifier, code } and <string lit> (effectively { path }) don't make sense. Should be a somewhat simple helper macro.

But with force_included and force_loaded as per-source options it might blow up a little, we'll see.

Probably can't avoid using a proc macro any longer.

[mmastrac]

I like the TOML version, though I think we can use this opportunity to make this API a bit easier to use. I would prefer to eliminate ExtensionFileSourceCode as it's really an internal detail.

  esm = [
    "02_init.js",
    // Node-style
    { path = "assert.ts", specifier = "node:assert" }
    // In this case the path is used to generate the "internal"-style specifier (ext:/...) 
    // -- requiring users to generate the `ext:` syntax only for this case seems odd to me
    { path = "buffer.ts", code = r#"..." }
    // Explicit specifier
    { specifier = "node:buffer", code = r#"..." }
    // loaded_during_snapshot can be set on any non-code item
    { path = "buffer.ts", loaded_during_snapshot = true },
  ]

I would recommend that we write a small macro that de-sugars the above TOML-style syntax to a const builder chain like so:

{ xyz = 123, foo = bar }

becomes

builder.xyz(123).foo(bar).build()

And for the default case just de-sugar to builder.path

builder.path("default").build()

This way we can add new fields to the builder without having the touch the macro. Validation can also happen at compile-time. There's a good chance we might need to integrate some sort of basic codegen solution into this for some of the core JS that needs it (ie: async op dispatch generates 0-N arguments versions).

The builder can be pre-populated with the dir above as well. Perhaps we should change that to dir = "xyz" instead?

I'd prefer to keep the proc macros out of it for as long as we can, because getting those done well and propagating errors is a ton of work.

[nayeemrmn]

I would prefer to eliminate ExtensionFileSourceCode as it's really an internal detail.

Yeah I wanted to support using the macro for https://github.com/denoland/deno/blob/0ec4feaee7a2dd442fc8955036999b550c9959ac/cli/build.rs#L321-L326, but I suppose we can write it as { path = "../runtime/js/99_main.js", specifier = "ext:cli/runtime/js/99_main.js" }.

Fair point about { path, code } being used to auto-generate the specifier in more regular cases.

Const builder is a lifesaver, I didn't think of that.

[nayeemrmn]

Big roadblock.... remember how concat!() only accepts string literals and not const string values, and that could be solved by replacing it with constcat::concat!(). I hit the same problem with include_str!() and this time there doesn't seem to be a viable replacement for reading files to static strings at compile time. It's something that would need to be a built-in and cheat the implementation https://github.com/rust-lang/rust/issues/53749#issuecomment-458038888. I think that kills the const builder solution.

There were more problems with it too:

• The code path to include_str!() wouldn't be conditionally compiled but depend on const booleans, so it would be called even with force_loaded though the value would be optimised out. Undesired but would have been acceptable.
• const fn build(self) -> ExtensionFileSource wouldn't let me declare self as a const value so I basically had to inline .build() in the macro with the builder fields published. It was slightly ugly.

[mmastrac]

Do you have a branch-in-progress that you can push and I can take a look at? We might be able to figure out a way through it.

[nayeemrmn]

https://github.com/nayeemrmn/deno_core/commits/js-source-objects

[mmastrac]

The code path to include_str!() wouldn't be conditionally compiled but depend on const booleans, so it would be called even with force_loaded though the value would be optimised out. Undesired but would have been acceptable.

TBH I think that's OK. While it'll technically add a bit of extra time to the build it, it should get pruned in the early IR optimization stages if we are very explicit about it being unused.

As long as the scripts don't end up in the final output, I'd prefer to lean towards ergonomics over build time optimization.

const fn build(self) -> ExtensionFileSource wouldn't let me declare self as a const value so I basically had to inline .build() in the macro with the builder fields published. It was slightly ugly.

I'm not sure what you mean here -- you should just be able to return ExtensionFileSource, shouldn't you?

[nayeemrmn]

TBH I think that's OK. While it'll technically add a bit of extra time to the build it, it should get pruned in the early IR optimization stages if we are very explicit about it being unused.

As long as the scripts don't end up in the final output, I'd prefer to lean towards ergonomics over build time optimization.

I agree it would have been borderline acceptable, but my point was that it limits use cases where you pass force_loaded because the file doesn't exist at build time. It would try to include it and fail.

const fn build(self) -> ExtensionFileSource wouldn't let me declare self as a const value so I basically had to inline .build() in the macro with the builder fields published. It was slightly ugly.

I'm not sure what you mean here -- you should just be able to return ExtensionFileSource, shouldn't you?

You have to be able to build static strings in there from the fields in self.

Anyway the biggest problem is include_str!(). We have to have to call that from syntax info because it can't accept const vars.