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

Really fix issue with detecting AMD-like imports #4010

Closed
wants to merge 1 commit into from
Closed

Really fix issue with detecting AMD-like imports #4010

wants to merge 1 commit into from

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Feb 16, 2020

#4009 fixed it for TypeScript, but the same detection can be performed on JavaScript.

#4009 fixed it for TypeScript, but
the same detection can be performed on JavaScript.
@kitsonk
Copy link
Contributor

kitsonk commented Feb 16, 2020

We want/need it in JavaScript, as there are situations where we are transpiling from AMD to ESM using the compiler, as mentioned in the PR.

@seishun
Copy link
Contributor Author

seishun commented Feb 16, 2020

Do such situations currently exist or is it pure theory?

@kitsonk
Copy link
Contributor

kitsonk commented Feb 16, 2020

They exist.

@seishun
Copy link
Contributor Author

seishun commented Feb 16, 2020

Please provide an example.

@kitsonk
Copy link
Contributor

kitsonk commented Feb 16, 2020

The only time JavaScript source files are created in the compiler is when checkJs is true in a TSConfig. When that is the case, the following would be converted to ESM:

define(["./foo.ts"], function (foo) {
  console.log(foo);
});

would emit as something like:

import * as foo from "./foo.ts";
console.log(foo);

The same applies for CommonJS modules:

const foo = require("./foo.ts");
console.log(foo);

I know this because I wrote the code, so it isn't speculation on my part. It is an intentional feature. The reason why there is an option in the TypeScript preprocess is specifically for this transpilation feature as well.

@seishun
Copy link
Contributor Author

seishun commented Feb 16, 2020

Could you provide more specific steps? I ran your CommonJS example in a directory with the following tsconfig.json:

{
  "compilerOptions": {
    "checkJs": true,
    "allowJs": true
  }
}

It didn't seem to be converted to ESM:

C:\Users\Nikolai\test-proj-delete>..\deno\target\debug\deno.exe test.js
error: Uncaught ReferenceError: require is not defined
► file:///C:/Users/Nikolai/test-proj-delete/test.js:1:13

1 const foo = require("./foo.ts");
              ^

    at file:///C:/Users/Nikolai/test-proj-delete/test.js:1:13

@kitsonk
Copy link
Contributor

kitsonk commented Feb 16, 2020

You need to pass the tsconfig.json as an argument to Deno. It does not automatically detect it.

@seishun
Copy link
Contributor Author

seishun commented Feb 16, 2020

C:\Users\Nikolai\test-proj-delete>..\deno\target\debug\deno.exe --config tsconfig.json test.js
error: Uncaught ReferenceError: require is not defined
► file:///C:/Users/Nikolai/test-proj-delete/test.js:1:13

1 const foo = require("./foo.ts");
              ^

    at file:///C:/Users/Nikolai/test-proj-delete/test.js:1:13

Do you get a different result?

@ry
Copy link
Member

ry commented Feb 24, 2020

This was addressed by @kitsonk in another way in 6431622

@ry ry closed this Feb 24, 2020
@seishun
Copy link
Contributor Author

seishun commented Feb 24, 2020

The issue is still there if checkJs is enabled.

tsconfig.json

{
  "compilerOptions": {
    "checkJs": true,
    "allowJs": true
  }
}

foo.ts

import "./test.js";

test.js

function define(foo) {}
define(["long"]);

..\deno\target\debug\deno.exe --config tsconfig.json foo.ts

Compile file:///C:/Users/Nikolai/test-proj-delete/foo.ts
error: Uncaught URIError: relative import path "long" not prefixed with / or ./ or ../ Imported from "file:///C:/Users/Nikolai/test-proj-delete/test.js"

@ry
Copy link
Member

ry commented Feb 24, 2020

I've opened #4110 with your report. Let's move the discussion there.

@seishun seishun deleted the fix-define-detect branch May 26, 2020 19:23
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.

3 participants