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

relative import path <node_module> not prefixed with #7997

Closed
cztomsik opened this issue Oct 16, 2020 · 9 comments
Closed

relative import path <node_module> not prefixed with #7997

cztomsik opened this issue Oct 16, 2020 · 9 comments
Labels
bug Something isn't working correctly cli related to cli/ dir

Comments

@cztomsik
Copy link

cztomsik commented Oct 16, 2020

I have some conditionals in my js files so that they work in both deno & latest nodejs, for example:

const Worker = globalThis.Worker ?? (await import("worker_threads")).Worker;

this worked fine few weeks ago but now I get this error and I'm not sure what to do about it:

error: relative import path "worker_threads" not prefixed with / or ./ or ../ Imported from "..."

I've quick fixed it with import(`${'worker_threads'}`) but it's ugly and I don't think people will want to do this for their deno+nodejs modules
(EDIT: quickfix actually doesn't work, it only fails later)

@kitsonk kitsonk added bug Something isn't working correctly cli related to cli/ dir labels Oct 16, 2020
@kitsonk
Copy link
Contributor

kitsonk commented Oct 16, 2020

Could you provide the version of Deno you are using and what you command line looks like?

I will try to make sure this is accounted for in #7225.

@cztomsik
Copy link
Author

1.4.6

image
image

@ghost
Copy link

ghost commented Oct 16, 2020

If it were to import the path, then that would indeed be the expected error, but it shouldn't run at all, as globalThis.Worker exists, right?

@cztomsik
Copy link
Author

you can see the [Function: Worker] there so it's a mystery (however, I don't think it is good idea to have that check even statically)

@ghost
Copy link

ghost commented Oct 16, 2020

you can see the [Function: Worker] there so it's a mystery

Ah, yes, I hadn't spotted that in the image you attached right above the source code.

Just to rule out any other possibilities, could you try checking your JavaScript output from TSC, to ensure that there's nothing wrong there?

I would assume it's the nullish coalescing operator that is broken here unless it's the import pseudo-function, so might I suggest using a more explicit ternary or if/else to cope until this is fixed? (unless you've already found another workaround that works)

@kitsonk
Copy link
Contributor

kitsonk commented Oct 16, 2020

It is a defect... we statically analyze the dependencies to try to ensure that we eagerly fetch all modules and have them ready. For dynamic import we should "try" so we don't get caught off guard, but we shouldn't log any errors/terminate when this occurs. This is a great example of why we shouldn't do it, and I wanted the details of what arguments were passed so I can make sure we can ensure that we have a test case that covers this issue.

As stated, I will try to address it in #7225, which should be closed for 1.5, so about two weeks away.

@ghost
Copy link

ghost commented Oct 17, 2020

... we statically analyze the dependencies to try to ensure that we eagerly fetch all modules...

I'd presume that it has to do with the module cache/graph updates?
From what it looks like, it fetches it at runtime, once the statement has started executing, like normal, but regardless of whether or not the executing branch needs it, is that correct?

I would've expected it to throw before code even ran if it were "static," but that doesn't seem to be the case, could you explain what it's doing with a bit more depth?

Maybe you don't need to totally stop doing it (if it can improve performance), but only import those that it can didn't throw?
Best of both worlds?

@kitsonk
Copy link
Contributor

kitsonk commented Oct 17, 2020

could you explain what it's doing with a bit more depth?

It is more than a few lines in an issue I am afraid. As I said, I know what is wrong, the behaviour experienced is undesired, and I am in a process of a major refactor of this part of the code under #7225 that should make the whole process more maintainable in the future, but the whole process has grown organically over the past 2.5 years and needs a good clean out. There are a handful of other issues that need to be address as part of this which I am taking care as well.

@tiagofrancafernandes
Copy link

Try this:
denoland/fresh#514 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly cli related to cli/ dir
Projects
None yet
Development

No branches or pull requests

3 participants