feat: deno vendor #13670

dsherret · 2022-02-14T23:56:48Z

Not part of this first pass:

Warning when dynamic import is not statically analyzable (Ability to tell about any dynamic import specifiers deno_graph could not statically analyze deno_graph#138)
Special handling of file system layout to take into account Window's 260 character path limit. I had some code that handled this, but it wasn't very good and I thought of a better solution. I'll open an issue and do another PR at a later time.
Module specifiers with different text in the same module that resolve to the same module aren't handled (Support multiple ranges for an import in the graph deno_graph#132) -- Not a big deal because this is probably super rare
Directories with different casing resolve to the same folder on case insensistive file systems. I will have an upcomming PR with the 260 char limit that fixes this (just added this to dnt here)
The specified import map is not combined with the generated import map. It might be hard to do this correctly because the base of the import map changes when going into the vendor folder. Right now it's not a big deal for people to manually update the created import map after.

Differences with proposal:

Uses vendor folder instead of _vendor by default (seemed like in the meeting people wanted vendor instead?).
Output is not dependent on the existing state of the vendor folder.
- Errors when the vendor folder exists and is non-empty. Allows using a --force flag to potentially overwrite the contents.
- No --info flag
- Specifying an import map via --import-map ... located within the output directory will error.
- Why: Vendor is already a complex feature and I think basing the vendor output on the existing state of the vendor folder would be very difficult to do. It would also be hard to fix bugs because we'd need to know the previous state of the vendor folder. I think it also might be difficult for some users to understand.

…ents though.

- Remove windows 260 limit code because it was terrible and I thought of a better solution that can come after the initial implementation. - Remove other code that's no longer used

- fix directory paths in import map (bug in url crate) - starting to add integration tests

lucacasonato · 2022-02-14T23:59:56Z

Module specifiers with different text in the same module that resolve to the same module aren't handled (denoland/deno_graph#132) -- Not a big deal because this is probably super rare

I agree not that important for a first pass, but I don't agree it's super rare. Often you see in deps.ts files when people re-export both types and symbols:

export { foo } from "https://example.com/";
export type { foo } from "https://example.com/";

dsherret · 2022-02-15T00:01:27Z

cli/flags.rs

@@ -2497,8 +2574,8 @@ mod tests {

  /// Creates vector of strings, Vec<String>
  macro_rules! svec {
-    ($($x:expr),*) => (vec![$($x.to_string()),*]);
-}
+    ($($x:expr),* $(,)?) => (vec![$($x.to_string()),*]);


Trailing comma support.

dsherret · 2022-02-15T00:06:34Z

cli/fs_util.rs

@@ -362,6 +362,32 @@ pub fn path_has_trailing_slash(path: &Path) -> bool {
  }
 }

+/// Gets a path with the specified file stem suffix.
+pub fn path_with_stem_suffix(path: &Path, suffix: &str) -> PathBuf {
+  if let Some(file_name) = path.file_name().map(|f| f.to_string_lossy()) {


to_string_lossy 🤷‍♂️

dsherret · 2022-02-15T00:07:24Z

cli/tests/integration/vendor_tests.rs

@@ -0,0 +1,331 @@
+// Copyright 2018-2022 the Deno authors. All rights reserved. MIT license.


Most of the tests aren't in this file. They'll be in cli/tools/vendor/build.rs. This is just the final integration tests.

dsherret · 2022-02-15T00:08:05Z

cli/tests/integration/vendor_tests.rs

+    .unwrap();
+  let output = deno.wait_with_output().unwrap();
+  assert_eq!(
+    String::from_utf8_lossy(&output.stderr).trim(),


Would be neat if we had a less verbose way of running the deno process and getting the output in the tests.

dsherret · 2022-02-15T00:18:33Z

@lucacasonato it will just be an issue where the module specifiers differ and don't match the file name like:

export { foo } from "https://example.com/mod";
export type { foo } from "https://example.com/redirect_to_mod";

Removes displaying the output directory in the message because it's a pain to test obviously.

lucacasonato · 2022-02-16T13:54:51Z

It'd be nice to add a little message after deno vendor succeeds:

$ deno vendor main.ts
Vendored x modules into vendor/ directory.

To use vendored modules, specify the `--import-map` flag when invoking deno subcommands:
  deno run -A --import-map=./vendor/import_map.json main.ts

cli/tools/vendor/mappings.rs

ry · 2022-02-16T15:32:54Z

cli/tools/vendor/mappings.rs

+    let mut text = from.make_relative(&to).unwrap();
+    if is_dir && !text.ends_with('/') && to.query().is_none() {
+      text.push('/');
+    }


Because of some of this nitty gritty heuristics, it would be good to have a little unit test for relative_path()

It's a bit of a pain to test this struct because it involves creating a module graph. I thought about extracting some of the functionality in here out, but it adds a bit of indirection. The logic is temporary workarounds and the code in here is tested transitively—if I delete either of the if statements or start flipping conditions then the vendor tests will fail.

ry · 2022-02-16T15:33:14Z

cli/tools/vendor/mappings.rs

+    &self,
+    from: &ModuleSpecifier,
+    to: &ModuleSpecifier,
+  ) -> String {


Might as well have a unit test for relative_specifier_text() too...

ry

Looks great. After fixing the issue that Luca discovered in the dotcom repo, I think this would be ready to land.

dsherret · 2022-02-16T16:05:37Z

@lucacasonato I added the message and fixed the issue seen on the dotcom repo.

lucacasonato

LGTM! All the code i tested now works as expected. 🎉

bartlomieju

LGTM, I have a few nitpicks, but they can be addressed after this PR lands

cli/tools/vendor/build.rs

bartlomieju · 2022-02-16T16:52:40Z

cli/tools/vendor/mappings.rs

+        },
+      )) = &module.maybe_types_dependency
+      {
+        // hack to tell if it's a x-typescript-types header


I don't get what this hack does :(

See denoland/deno_graph#133

The hack is to check if the range start and end in the file is zero.

cli/tools/vendor/mod.rs

littledivy

LGTM! Tested it on a bunch of projects, its perfect!

dsherret added 26 commits February 3, 2022 18:25

Committing for backup purposes.

fadf48a

Add copyrights.

8532d19

Merge branch 'main' into deno_vendor

9699a5c

Starting to add text changes.

e0db996

Somewhat working.

383a962

Slight fix.

85ab7ed

Better import map and leave output files the same more

31c4996

Merge remote-tracking branch 'upstream/main' into deno_vendor

e5fd1f4

Small fixes. Need to resolve the todos... might be a bit of work.

591042d

Lots of bugs, but slowly getting there. Will need deno_graph improvem…

5e6022a

…ents though.

- Inspect the source code itself to get the specifier text

52c7818

- Remove windows 260 limit code because it was terrible and I thought of a better solution that can come after the initial implementation. - Remove other code that's no longer used

Basic testing infrastructure.

6a54b35

Fix more issues. It seems to be working decently now.

677f492

Maintain casing of extensions when able

8317c87

Merge branch 'main' into deno_vendor

855dbe4

More tests.

cf746ae

- derived import map and functionality test

a981b53

- fix directory paths in import map (bug in url crate) - starting to add integration tests

Merge remote-tracking branch 'upstream/main' into deno_vendor

bd77405

Support x-typescript-types headers

2974cd1

Fix and add tests for default export detection.

fe1413f

Add flags test.

36bc916

More integration tests and fixes.

9762a40

Add sub command description

b77ddb5

Add test for an existing import map.

8c16515

Tests for dynamic imports

4286ffd

Add support for more flags.

8d10e4d

Updates based on self review.

72fb9b4

dsherret commented Feb 15, 2022

View reviewed changes

dsherret requested a review from bartlomieju as a code owner February 15, 2022 00:34

dsherret requested a review from kitsonk February 15, 2022 00:34

Maybe fix mac.

3d097a5

dsherret force-pushed the deno_vendor branch from 26ecbea to 3d097a5 Compare February 15, 2022 01:21

dsherret added 2 commits February 14, 2022 20:55

Fix mac issue on a mac

f04366c

Probably fix on all operating systems.......

4056996

Removes displaying the output directory in the message because it's a pain to test obviously.

lucacasonato added this to the 1.19.0 milestone Feb 15, 2022

ry reviewed Feb 16, 2022

View reviewed changes

cli/tools/vendor/mappings.rs Show resolved Hide resolved

ry reviewed Feb 16, 2022

View reviewed changes

dsherret added 2 commits February 16, 2022 10:40

Referencing a different origin should add entry to "imports"

d147900

Add message on success.

8b10b6a

dsherret added 3 commits February 16, 2022 11:10

Merge branch 'main' into deno_vendor

b3b98cc

Fix clippy issues on main.

326f399

Update for new deno_graph changes.

dd8d561

dsherret requested review from crowlKats, lucacasonato and littledivy as code owners February 16, 2022 16:18

lucacasonato approved these changes Feb 16, 2022

View reviewed changes

bartlomieju approved these changes Feb 16, 2022

View reviewed changes

Add context to canonicalize.

bfa2ee9

littledivy reviewed Feb 16, 2022

View reviewed changes

cli/tools/vendor/mod.rs Outdated Show resolved Hide resolved

littledivy approved these changes Feb 16, 2022

View reviewed changes

Use first non-remote entry point in output message.

90bce37

dsherret merged commit b98afb5 into denoland:main Feb 16, 2022

dsherret deleted the deno_vendor branch February 16, 2022 18:14

dsherret mentioned this pull request Feb 16, 2022

deno vendor does not work with existing import map with bare specifier mapping #13697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: deno vendor #13670

feat: deno vendor #13670

dsherret commented Feb 14, 2022 •

edited

lucacasonato commented Feb 14, 2022

dsherret Feb 15, 2022

dsherret Feb 15, 2022

dsherret Feb 15, 2022

dsherret Feb 15, 2022

dsherret commented Feb 15, 2022

lucacasonato commented Feb 16, 2022

ry Feb 16, 2022

dsherret Feb 16, 2022

ry Feb 16, 2022

ry left a comment •

edited

dsherret commented Feb 16, 2022

lucacasonato left a comment

bartlomieju left a comment

bartlomieju Feb 16, 2022

dsherret Feb 16, 2022

littledivy left a comment

		@@ -0,0 +1,331 @@
		// Copyright 2018-2022 the Deno authors. All rights reserved. MIT license.

feat: deno vendor #13670

feat: deno vendor #13670

Conversation

dsherret commented Feb 14, 2022 • edited

lucacasonato commented Feb 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsherret commented Feb 15, 2022

lucacasonato commented Feb 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment • edited

Choose a reason for hiding this comment

dsherret commented Feb 16, 2022

lucacasonato left a comment

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledivy left a comment

Choose a reason for hiding this comment

dsherret commented Feb 14, 2022 •

edited

ry left a comment •

edited