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

Import map not working with Worker #6675

Closed
wongjiahau opened this issue Jul 7, 2020 · 22 comments
Closed

Import map not working with Worker #6675

wongjiahau opened this issue Jul 7, 2020 · 22 comments
Labels
cli related to cli/ dir feat new feature (which has been agreed to/accepted)

Comments

@wongjiahau
Copy link
Contributor

wongjiahau commented Jul 7, 2020

Steps to reproduce

  1. Create the following files under the same directory:

import_map.json

{
   "imports": {
      "fmt/": "https://deno.land/[email protected]/fmt/"
   }
}

color.ts

import { red } from "fmt/colors.ts";

console.log(red("hello world"));

main.ts

new Worker(new URL('./color.ts', import.meta.url).href, {type: 'module'})
  1. Run this command:
deno run --allow-read --importmap=import_map.json --unstable main.ts

Expected output

hello world

Actual output

error: Uncaught Error: relative import path "fmt/colors.ts" not prefixed with / or ./ or ../ Imported from "file:///color.ts"

Environment

deno 1.1.3
@ry ry added bug Something isn't working correctly cli related to cli/ dir labels Jul 8, 2020
@wongjiahau
Copy link
Contributor Author

@ry If I want to fix this which files should I look at?

@bartlomieju
Copy link
Member

According to import maps spec they are not affecting workers. So rather than a bug this would be a feature and would be Deno specific. It could be added to non-standard deno option in Worker constructor.

@wongjiahau
Copy link
Contributor Author

@bartlomieju I see, so is it something like ‘importMap: URL’ option for Worker?

@ry ry added feat new feature (which has been agreed to/accepted) and removed bug Something isn't working correctly labels Jul 9, 2020
@qballer
Copy link

qballer commented Jul 9, 2020

Having import map loading different context of modules could introduce interesting use cases for stuff like A/B testing

@bartlomieju
Copy link
Member

@wongjiahau that would be something like:

declare class Worker extends EventTarget {
    constructor(
      specifier: string,
      options?: {
        type?: "classic" | "module";
        name?: string;
        deno?: {
            importMap?: string
        };
      }
    );
  }

@wongjiahau
Copy link
Contributor Author

@bartlomieju Can the import map represents a remote file? for example

https://deno.land/x/import_map.json

@bartlomieju
Copy link
Member

@wongjiahau not at the moment, there is a PR for it (#5849), but I haven't reviewed it yet

@wongjiahau
Copy link
Contributor Author

@bartlomieju Ok, so importMap can only be either relative or absolute file path right?

@bartlomieju
Copy link
Member

@wongjiahau that's correct

@wongjiahau
Copy link
Contributor Author

@bartlomieju Thanks I will create a PR based on the direction you provided

@bartlomieju
Copy link
Member

From https://github.com/WICG/import-maps#installation

What do we do in workers? Probably new Worker(someURL, { type: "module", importMap: ... })? 
Or should you set it from inside the worker? 
Should dedicated workers use their controlling document's map, either by default or always? Discuss in #2.

Related discussion: WICG/import-maps#2

Let's wait until proper solution is decided in the spec repo before implementing it in Deno.

@abourdin
Copy link

abourdin commented Mar 9, 2021

Is there currently any way to test a web server that is using an import map until this is implemented in Deno?

@rracariu
Copy link

rracariu commented May 8, 2021

Until there is an standardized way for import maps in Workers, ca we please have it under the deno options?
ATM this issue throws a wrench into applications designed to work with import maps...

@oscarotero
Copy link
Contributor

oscarotero commented Sep 26, 2021

I guess the spec will take a long time to decide a final solution, and import maps is something very popular and used right now in Deno, so I believe Deno should provide a way to handle this. The importMaps key under deno proposed in #6675 (comment) seems the obvious choice.

@remyrylan
Copy link

Is there any possibility to get this implemented at least behind the unstable flag for now?

@kitsonk
Copy link
Contributor

kitsonk commented Dec 26, 2021

No, because we want to continue to align to the web standard. If the web standard is strongly considering establishing a proposal, then we might consider it. Best thing to do is continue to champion something with WHATWG.

@oscarotero
Copy link
Contributor

Currently, Workers have the deno key to configure the permissions and whether the Deno namespace is enabled or not.

Maybe a solution, that I believe won't conflict with what WHATWG decide in the future is include a way to set the deno config file (deno.json), so the import map would be taken from that file.

This not only allows to configure the import maps but also other things like TypeScript options, etc. A proposal:

const worker = new Worker(new URL("./worker.js", import.meta.url).href, {
  type: "module",
  deno: {
    permissions: "none",
    config: "inherit" // Or the path to a different file
  },
});

@NyanHelsing
Copy link

it might be possible to write a swc plugin and run the files through swc before running them in deno.

the plugin would basically visit all the named imports/exports and replace the specifier as per the import_map.

@NyanHelsing
Copy link

NyanHelsing commented Feb 13, 2023

it might be possible to write a swc plugin and run the files through swc before running them in deno.

the plugin would basically visit all the named imports/exports and replace the specifier as per the import_map.

ChatGPT says we can do this:

Qs:

  • how to get the imprt map into the constructor? theres config that can get passed to swc plugn, need to answer that q.
  • need to check implementation is actually correct/functional... it's hopefully at least close.
  • will this work in node.js too?
use std::collections::HashMap;
use string_cache::Atom;
use swc_core::ecma::{
    ast::{ImportDecl, NamedExport, Program},
    transforms::testing::test,
    visit::{as_folder, FoldWith, VisitMut, VisitMutWith},
};
use swc_core::plugin::{plugin_transform, proxies::TransformPluginProgramMetadata};

pub struct TransformVisitor {
    import_map: HashMap<String, String>,
}

impl TransformVisitor {
    pub fn new(import_map: HashMap<String, String>) -> Self {
        Self { import_map }
    }
}

impl VisitMut for TransformVisitor {
    fn visit_mut_import_decl(&mut self, decl: &mut ImportDecl) {
        decl.visit_mut_children_with(self);
        for (key, value) in &self.import_map {
            if decl.src.value.starts_with(key) {
                decl.src = Box::new(Atom::from(value.to_owned() + &decl.src.value[key.len()..]).into());
                break;
            }
        }
    }
    
    fn visit_mut_named_export(&mut self, decl: &mut NamedExport) {
        decl.visit_mut_children_with(self);
        if let Some(src) = &decl.src {
            for (key, value) in &self.import_map {
                if src.value.starts_with(key) {
                    decl.src = Some(Box::new(Atom::from(value.to_owned() + &src.value[key.len()..]).into()));
                    break;
                }
            }
        }
    }
}

@jespertheend
Copy link
Contributor

It seems like this is working fine for me? It seems to pick up the import map from my deno.json. I'm using the importMap property, not sure about imports.

@nounder
Copy link

nounder commented Jul 26, 2024

Import maps in workers function properly here, too.

deno 1.45.2 (release, aarch64-apple-darwin)
v8 12.7.224.12
typescript 5.5.2

@marvinhagemeister
Copy link
Contributor

Marking as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir feat new feature (which has been agreed to/accepted)
Projects
None yet
Development

Successfully merging a pull request may close this issue.