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

Add text and binary module types #1025

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

SpaceManiac
Copy link

@SpaceManiac SpaceManiac commented Jan 3, 2025

Pursuant to denoland/deno#20374 and denoland/deno#25354.

  • Add RequestedModuleType::as_str convenience method.
  • Add RequestedModuleType parameter to prepare_load to mirror load.
    • This is needed to let text and binary loading skip parsing as JS syntax, which happens during prepare_load. Otherwise any file that looks-like JS or JSON based on file extension or Content-Type header can't be imported as text or binary.
  • Add and implement ModuleType::Text and Binary.
    • Meant to eventually be used like:
      import myText from "./deno.jsonc" with { type: "text" };
      import myBinary from "./logo.png" with { type: "binary" };
      myText satisfies string;
      myBinary satisfies ArrayBuffer;
    • Question: Does this belong in deno_core? It seems broadly useful, but maybe you want to keep core closer to the ECMAScript standard and implement these in the deno CLI embedding. I think this would be the first module type not to go in core if so.
    • Question: Is "binary" the right name, or do you want "bytes"?
    • Question: Is it okay that ArrayBuffer is mutable? If not, can we hackily make the import return something immutable, or must we block on standardization and v8 implementation?

Fully implementing this feature will also involve changes to deno_graph and deno, and a one-line change to deno_doc.

  • Might need tests.

@CLAassistant
Copy link

CLAassistant commented Jan 3, 2025

CLA assistant check
All committers have signed the CLA.

@bartlomieju
Copy link
Member

I don't think these changes are necessary, if we decide to do it in Deno, the prepare_module_load hook can figure out what to do with the file. That said, there's no plan to address these two issues at the moment, but that might change later in the quarter.

@SpaceManiac
Copy link
Author

SpaceManiac commented Jan 3, 2025

the prepare_module_load hook can figure out what to do with the file.

The trouble I ran into here is that dynamic imports are handled directly by deno_core, call RecursiveModuleLoad::prepare which (without this patch) throws away the RequestedModuleType, and then deno's ModuleLoader impl and therefore ModuleLoadPreparer::prepare_module_load has no knowledge of whether a URL was imported with type: "text" or not. Then it goes down and tries to parse Unknown or JSON files per JavaScript syntax which of course fails on arbitrary text files, or on files which have .json extension (and thus media-type) but are actually JSONC.

@petamoriken
Copy link
Contributor

IMHO, this feature should not be included because it is still a proposal in the discussion stage in the web standards.
whatwg/html#9444

At least, we need to make sure that there is no opposition in the standard position of the three browsers.

@petamoriken
Copy link
Contributor

Or, it requires the unstable flag.

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.

4 participants