feat(compile): unstable npm and node specifier support #19005
Conversation
| } | ||
| } | ||
|
|
||
| // todo(dsherret): we should store this more efficiently in the binary |
There was a problem hiding this comment.
I did zero optimizations for storing this efficiently in the binary. We could store this in a binary format or shorten the property names in the json.
There was a problem hiding this comment.
I think this is perfectly reasonable for the first pass
| } | ||
|
|
||
| #[derive(Clone)] | ||
| struct FileBackedVfsFile { |
There was a problem hiding this comment.
Something we may want to consider is storing the files compressed, then decompressing them on open.
There was a problem hiding this comment.
Sounds like a good idea, but we should benchmark it - might not be worth added complexity and startup time 👍
There was a problem hiding this comment.
I could be nitpicky about various things could land prior to this PR, but I'm not gonna do that :)
Tested this locally in a few projects and I was even able to create an eslint binary in seconds. This is gonna be a great feature.
I want to give it another pass and get more eyes on it before approving, but overall fantastic work. Weeks of refactors and cleanups are paying off.
| let _trailer_pos = bufreader | ||
| .seek(SeekFrom::End(-(TRAILER_SIZE as i64))) | ||
| .await?; | ||
| let mut trailer = [0; TRAILER_SIZE]; | ||
| bufreader.read_exact(&mut trailer).await?; | ||
| let (magic_trailer, rest) = trailer.split_at(8); | ||
| if magic_trailer != MAGIC_TRAILER { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| let (eszip_archive_pos, rest) = rest.split_at(8); | ||
| let metadata_pos = rest; | ||
| let eszip_archive_pos = u64_from_bytes(eszip_archive_pos)?; | ||
| let metadata_pos = u64_from_bytes(metadata_pos)?; | ||
| let metadata_len = trailer_pos - metadata_pos; | ||
| let trailer = match Trailer::parse(&trailer)? { | ||
| None => return Ok(None), | ||
| Some(trailer) => trailer, | ||
| }; |
There was a problem hiding this comment.
Unrelated to this PR, but do you see any other faster way of checking if this is deno compile binary? Asking in the context of startup time - seems like quite a lot of work to do before we start executing code in deno run subcommand.
There was a problem hiding this comment.
Probably we should refactor the code more to be able to have slim binaries like before.
| // DO NOT include the user's registry url as it may contain credentials, | ||
| // but also don't make this dependent on the registry url | ||
| let registry_url = self.npm_api.base_url(); | ||
| let root_path = self.npm_cache.registry_folder(registry_url); | ||
| let mut builder = VfsBuilder::new(root_path); |
There was a problem hiding this comment.
I don't get what's happening here - if we're not depending on this info how do we know we use the correct one?
There was a problem hiding this comment.
This stores the information relative to a base like C:\Users\david\AppData\Local\deno\npm\registry.npmjs.org\ (so it doesn't include the registry url). Then when loading in cli/standalone/mod.rs, it uses the dummy registry https://localhost/ for the npm registry url (which should never be requested) and that gets mapped appropriately in the vfs to a folder that takes that into account.
There was a problem hiding this comment.
This is going to get more challenging once we support multiple npm registries...
| } | ||
|
|
||
| #[async_trait::async_trait(?Send)] | ||
| impl FileSystem for DenoCompileFileSystem { |
| maybe_binary_npm_command_name: NpmPackageReqReference::from_specifier( | ||
| main_module, | ||
| ) | ||
| .ok() | ||
| .map(|req_ref| npm_pkg_req_ref_to_binary_command(&req_ref)), |
There was a problem hiding this comment.
Thanks for fixing this! Also: really strange formatting here 😓
| } | ||
| } | ||
|
|
||
| // todo(dsherret): we should store this more efficiently in the binary |
There was a problem hiding this comment.
I think this is perfectly reasonable for the first pass
| } | ||
|
|
||
| #[derive(Clone)] | ||
| struct FileBackedVfsFile { |
There was a problem hiding this comment.
Sounds like a good idea, but we should benchmark it - might not be worth added complexity and startup time 👍
| // get the virtual fs | ||
| let (dest_path, virtual_fs) = into_virtual_fs(builder, &temp_dir); | ||
|
|
||
| assert_eq!(read_file(&virtual_fs, &dest_path.join("a.txt")), "data"); |
This is the initial support for npm and node specifiers in `deno compile`. The npm packages are included in the binary and read from it via a virtual file system. This also supports the `--node-modules-dir` flag, dependencies specified in a package.json, and npm binary commands (ex. `deno compile --unstable npm:cowsay`) Closes #16632
|
Now that this is merged, does this mean that support for npm specifiers could be coming to Deno Deploy? |
Yes, we are working on it. |
Great news @bartlomieju. Really impressed with the teams persistent on ironing out these kind of issues and making Deno a great experience for both greenfield projects, and teams that need to live with npm to some degree for the forsable future. 🙂 |
This is the initial support for npm and node specifiers in
deno compile. The npm packages are included in the binary and read from it via a virtual file system. This also supports the--node-modules-dirflag, dependencies specified in a package.json, and npm binary commands (ex.deno compile --unstable npm:cowsay)Closes #16632