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

BREAKING: disallow static import of local modules from remote modules #5050

Merged

Conversation

bartlomieju
Copy link
Member

This commit changes module loading logic to disallow statically import
local module (file:// scheme) from remote modules (http://, https://
schemes).

This commit changes module loading logic to disallow statically import
local module (file:// scheme) from remote modules (http://, https://
schemes).
@bartlomieju bartlomieju changed the title BREAKING: disallow static import of local modules from remote modules [WIP] BREAKING: disallow static import of local modules from remote modules May 2, 2020
@bartlomieju
Copy link
Member Author

Ref #3401

@lucacasonato
Copy link
Member

lucacasonato commented May 2, 2020

This would break some nice use cases. One that comes to mind is filesystem based web server routes.

@bartlomieju
Copy link
Member Author

This would break some nice use cases. One that comes to mind is filesystem based web server routes.

How so? Dynamic imports are still working.

@lucacasonato
Copy link
Member

Oh whoops, I misread the title - I missed the static part. Then this seems like a really nice change :-D

@@ -0,0 +1,2 @@
[WILDCARD]
Remote module are not allowed to statically import local modules. Use dynamic import instead.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of .js-only imports the error is very sparse and has no stack trace...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better than nothing

@bartlomieju bartlomieju changed the title [WIP] BREAKING: disallow static import of local modules from remote modules BREAKING: disallow static import of local modules from remote modules May 2, 2020
@bartlomieju bartlomieju requested a review from ry May 2, 2020 11:58
Comment on lines +115 to +120
// TODO(bartlomieju): duplicated from `state.rs::ModuleLoader::load` - deduplicate
// Verify that remote file doesn't try to statically import local file.
if let Some(referrer) = ref_specifier_.as_ref() {
let referrer_url = referrer.as_url();
match referrer_url.scheme() {
"http" | "https" => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cruft will be removed during TS compiler refactor

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bartlomieju bartlomieju merged commit 2872b36 into denoland:master May 2, 2020
@bartlomieju bartlomieju deleted the remove_local_static_imports branch May 2, 2020 13:51
SASUKE40 pushed a commit to SASUKE40/deno that referenced this pull request May 7, 2020
…denoland#5050)

This commit changes module loading logic to disallow statically import
local module (file:// scheme) from remote modules (http://, https://
schemes).
@vovacodes
Copy link

vovacodes commented Sep 27, 2020

@bartlomieju I've run into a nice (IMO) use-case that this logic prevents: I'm using an import map to remap one of my transitive dependencies to a local file, but when trying to bundle my app it fails with the error:

error: Remote modules are not allowed to statically import local modules. Use dynamic import instead.
Imported from "https://cdn.skypack.dev/-/react-three-fiber@v5.0.1-s8hXZQQnJMVusvGcRf04/dist=es2020/react-three-fiber.js:13"

Can I work it around somehow?

PS I submitted the issue: #7723, it's probably better to keep the discussion there

@bartlomieju
Copy link
Member Author

@vovaguguiev currently there is no way. Would you mind opening an issue with your use case?

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.

4 participants