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

Allow the use of provided sources while still resolving remote modules #9866

Closed
EliteScientist opened this issue Mar 22, 2021 · 16 comments
Closed
Assignees
Labels
public API related to "Deno" namespace in JS suggestion suggestions for new features (yet to be agreed)

Comments

@EliteScientist
Copy link

EliteScientist commented Mar 22, 2021

This posted before I could fill out the form...

connectors:///conn-deps:/connectors.ts is provided by an import map for @j2inn/connectors and a source entry in the EmitOptions.

This is loading fine. The issue is that the TS code has an import to "https://deno.land/[email protected]/uuid/v4.ts" which fails

the deno command is executed as:
deno test --allow-plugin --allow-read --allow-net --allow-write --unstable --allow-env --allow-run --config=core\tsconfig.json --importmap=core\import-map.json %*

test [Connector] Create connector from Record ... thread 'main' panicked at 'Error 'Unable to output bundle during Graph::bundle().' contains boxed error of unknown type:
Error { context: "Unable to output bundle during Graph::bundle().", source: load_transformed failed

Caused by:
0: failed to analyze module
1: failed to resolve https://deno.land/[email protected]/uuid/v4.ts from connectors:///conn-deps:/connectors.ts
2: The graph is missing a dependency.
Specifier: https://deno.land/[email protected]/uuid/v4.ts from connectors:///conn-deps:/connectors.ts }
Error { context: "load_transformed failed", source: failed to analyze module

Caused by:
0: failed to resolve https://deno.land/[email protected]/uuid/v4.ts from connectors:///conn-deps:/connectors.ts
1: The graph is missing a dependency.
Specifier: https://deno.land/[email protected]/uuid/v4.ts from connectors:///conn-deps:/connectors.ts }
Error { context: "failed to analyze module", source: failed to resolve https://deno.land/[email protected]/uuid/v4.ts from connectors:///conn-deps:/connectors.ts

Caused by:
The graph is missing a dependency.
Specifier: https://deno.land/[email protected]/uuid/v4.ts from connectors:///conn-deps:/connectors.ts }
Error { context: "failed to resolve https://deno.land/[email protected]/uuid/v4.ts from connectors:///conn-deps:/connectors.ts", source: The graph is missing a dependency.
Specifier: https://deno.land/[email protected]/uuid/v4.ts from connectors:///conn-deps:/connectors.ts }
MissingDependency(Url { scheme: "connectors", host: None, port: None, path: "/conn-deps:/connectors.ts", query: None, fragment: None }, "https://deno.land/[email protected]/uuid/v4.ts")', cli\errors.rs:35:7
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

@EliteScientist
Copy link
Author

I am using version 1.8.1

@EliteScientist
Copy link
Author

Upgraded to version 1.8.2 and am seeing the same issue.

@EliteScientist EliteScientist changed the title Deno.emit() fails to grab http resources Deno.emit() fails to grab http resources when bundle="esm" Mar 26, 2021
@EliteScientist
Copy link
Author

EliteScientist commented Mar 29, 2021

Here is test code that reproduces the issue every time: I run this with:
deno test -A --unstable bugTest.ts

import { assert, assertEquals, assertThrowsAsync } from "https://deno.land/[email protected]/testing/asserts.ts";



Deno.test("[BugTest] First Test", async () =>
{
	let emissionConfig:Deno.EmitOptions		= 
	{
		bundle: "esm",
		check: false,
		sources: {
			"test:///test.ts": `import {generate} from "https://deno.land/std/uuid/v4.ts"; export default class MyTest {}`
		},
		compilerOptions:
		{
			"strict": true,
			"noImplicitAny": true,
			"noImplicitThis": true,
			"alwaysStrict": true,
			"strictBindCallApply": true,
			"strictFunctionTypes": true,
			"strictPropertyInitialization": true,
			"strictNullChecks": true,
			"allowJs": false,
			"checkJs": false,
			"experimentalDecorators": true,
			"importsNotUsedAsValues": "preserve"
		}
	};

	const {files, diagnostics}	= await Deno.emit("test:///test.ts", emissionConfig);
	console.log(`Received Source: ${files}`);
});

@kitsonk kitsonk added the needs investigation requires further investigation before determining if it is an issue or not label Mar 29, 2021
@kitsonk kitsonk self-assigned this Mar 29, 2021
@Pierstoval
Copy link

I also have the same issue when trying to build Tauri with Deno.

All files that use Deno are in the PR, like my build.ts script to build it, the import map, etc., so you can reproduce the bug by checking out the PR. Just clone this repo, check out the PR (or the branch on my fork), go to the tooling/api/ directory and run make emit.

I think it's worth noting that the deno bundle command works properly (I added it as a make bundle command in the Makefile I use in the above PR), but I want to use Deno.emit() to build to esm, but adding bundle: "esm" or bundle: "iife" makes the command throw an error 😕

Friendly ping @kitsonk since he assigned himself to this issue?

@EliteScientist
Copy link
Author

Yes I'd also like to mention that Deno.emit itself does work without having to load any http resources. What I did was use fetch to grab the HTTP resources then add them to the sources table which allowed me to temporarily continue to work.

@kitsonk
Copy link
Contributor

kitsonk commented Apr 22, 2021

I'm not sure @Pierstoval issue is the same as @EliteScientist. I hadn't paid attention to the example for @EliteScientist. You are specifying sources. The inline documentation for sources make it clear:

A record of sources to use when doing the emit. If provided, Deno will use these sources instead of trying to resolve the modules externally.

So in the example, it is "working as expected". If sources were not supplied and there were issues, then that would require investigation. Is there a reproduction that does not include sources?

@Pierstoval the CI builds failures on the linked issue don't seem to be related to Deno at all. Could you provide more insight into what was actually failing?

@kitsonk kitsonk added needs info needs further information to be properly triaged and removed needs investigation requires further investigation before determining if it is an issue or not labels Apr 22, 2021
@EliteScientist
Copy link
Author

@kitsonk sources was simply specified to make this a copy/paste test without having multiple files. In my code base, it was loaded from a .ts file. The code from sources has an import requesting "https://deno.land/std/uuid/v4.ts" that is where it fails.

While analyzing the issue, I found that if you try to bundle any file that imports "http" resources, it will throw the bundle error. It does the same thing when trying to import file resources.

What I have do so far, as a workaround, was use sources. I use fetch to grab known http resources and the deno read file ops to grab file resources to pass to Deno.emit.

For my project, this is a temporary solution as the http resources are unknown.

Now using Deno.emit to compile user submitted code that also gets executed could also pose a security issue when allowed to import any http resource. It would be nice if we could have a whitelist of domains that we can pass to the Deno.emit function to secure this.

@kitsonk
Copy link
Contributor

kitsonk commented Apr 23, 2021

Ok, I have looked into this and can't seem to reproduce.

example.ts

import { generate } from "https://deno.land/std/uuid/v4.ts";

console.log(generate());

deno_emit.ts

const result = await Deno.emit("./example.ts", {
  bundle: "esm",
});

console.log(result);
$ deno --version
deno 1.9.1 (release, x86_64-apple-darwin)
v8 9.1.269.5
typescript 4.2.2

$ deno run --unstable --allow-read --allow-net deno_emit.ts
Check file:///Users/kitsonk/github/test/deno_emit.ts
Download https://deno.land/std/uuid/v4.ts
Warning Implicitly using latest version (0.94.0) for https://deno.land/std/uuid/v4.ts
Download https://deno.land/[email protected]/uuid/v4.ts
Download https://deno.land/[email protected]/uuid/_common.ts
{
  diagnostics: [],
  files: {
    "deno:///bundle.js": "function bytesToUuid(bytes) {\n    const bits = [\n        ...bytes\n    ].map((bit)=>{\n        const s..."
  },
  ignoredOptions: null,
  stats: [
    [ "Files", 56 ],
    [ "Nodes", 36065 ],
    [ "Identifiers", 13515 ],
    [ "Symbols", 9586 ],
    [ "Types", 4788 ],
    [ "Instantiations", 1142 ],
    [ "Parse time", 15 ],
    [ "Bind time", 34 ],
    [ "Check time", 224 ],
    [ "Emit time", 9 ],
    [ "Total TS time", 282 ],
    [ "Compile time", 290 ]
  ]
}

Again, in the example above, generate was imported but not used in the module it was imported into, which means the bundler would have elided it, and therefore not required the module to be available.

@Pierstoval
Copy link

@Pierstoval the CI builds failures on the linked issue don't seem to be related to Deno at all. Could you provide more insight into what was actually failing?

The linked PR is still an experiment and isn't tied to the build process of the project itself, so CI is not to be taken in account. You need to actually clone the repo for that.

I managed to make Deno.emit() work, but as you can see, it needs a bit more configuration: https://github.com/Pierstoval/tauri/blob/af574276db1343f7c890c357953a9656456e4d2e/tooling/api/build.ts

I think this might have been an issue with the compilerOptions but I'm still investigating 😕

@EliteScientist
Copy link
Author

@kitsonk I've upgraded to 1.9.2 and I still get the same error when the original source is defined in sources.

With the entry code defined in sources, I'm unable to import http and file resources.

@kitsonk
Copy link
Contributor

kitsonk commented Apr 23, 2021

Ok, maybe I am again missing something... When you supply sources, you have to provide all the sources, as it no longer externally resolves modules, as per the inline documentation. There is not currently a "mixed" mode where you can supply some sources and read others from disk or the network.

A record of sources to use when doing the emit. If provided, Deno will use these sources instead of trying to resolve the modules externally.

Is there someway the documentation for the option sources be made clearer?

The best way to emit/bundle something that you want to resolve sources externally is to write it to disk.

@EliteScientist
Copy link
Author

@kitsonk Ahh ok. That makes sense. What I was trying to do was load code from a zip file, compile it, and run it.

I interpreted that "If provided, Deno will use these sources instead of trying to resolve the modules externally" meant that the modules defined in sources would have priority over external imports. I thought it would still resolve external modules.

Maybe state that "If defined, this disables all external module resolution. The compiler will only be able to resolve resources defined in the sources table."

Do you think it would be useful to have a module resolution function? Instead of defining a table, when requested I can return source code for the module. At this point I can create my own security mechanism for imports.

@kitsonk kitsonk changed the title Deno.emit() fails to grab http resources when bundle="esm" Allow the use of provided sources while still resolving remote modules Apr 25, 2021
@kitsonk kitsonk added public API related to "Deno" namespace in JS suggestion suggestions for new features (yet to be agreed) and removed needs info needs further information to be properly triaged labels Apr 25, 2021
@kitsonk
Copy link
Contributor

kitsonk commented Apr 25, 2021

@EliteScientist I can see the use case...

I was doing some thinking about it, and I am thinking that the way to solve (and potentially some other use cases like denoland/deno_emit#41 is to provide emit hooks that would allow user code to be able to resolve module specifiers and load sources. This would be similar to the resolve and load hooks that are part of AMD and SystemJS (as well as other module loaders) and would help cater towards advance use cases like compiler plugins.

In the case of a "mixed" mode, the "load" plugin would determine if the module is something that the source is provided for, or otherwise resolved as normal by Deno. I would expect it would look something like this:

Deno.emit("test:///test.ts", {
  load(specifier) {
    if (specifier.startsWith("test:///")) {
      return `export * from "https://deno.land/x/example/mod.ts";`
    }
  })
});

I need to work more details, but I think this would be preferable to having a load of complex options to indicate what should and shouldn't be internal versus external.

@EliteScientist
Copy link
Author

Awesome. It would be nice of load was also async. So we could do something like this:

Deno.emit("test:///test.ts",
{
    load: async (specifier) =>
    {
        if (isLocal)
            return await Deno.readFileAsync("filePath.ts");
        else
        if (isRemote)
            return await fetch("http://someHost/some/path.ts");
    }
}

@oscarotero
Copy link
Contributor

Deno graph (https://deno.land/x/[email protected]) has really nice options allowing to customize the way it loads, checks, resolves, etc the dependencies. I believe similar options for Deno.emit whould be great.

@lucacasonato
Copy link
Member

Deno.emit is no more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
public API related to "Deno" namespace in JS suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

No branches or pull requests

5 participants