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

Default map should be always preferred over import map given by user #164

Closed
aiotter opened this issue Jan 19, 2022 · 11 comments
Closed

Default map should be always preferred over import map given by user #164

aiotter opened this issue Jan 19, 2022 · 11 comments

Comments

@aiotter
Copy link
Contributor

aiotter commented Jan 19, 2022

lume/ci.ts

Line 88 in 89a7954

map.imports = { ...map.imports, ...resolvedMap.imports };

Shouldn't this be opposite?
{ ...resolvedMap.imports, ...map.imports } seems better.

I spent 2-3 hours resolving the following error, and that was because of mismatch between the import map I specified (lume==1.4.2) and the real Lume version(lume==1.4.3).

TypeError: Cannot redefine property: Symbol(Symbol.hasInstance)
    at (https://deno.land/x/deno_dom@v0.1.20-alpha/src/api.ts:58:8)

I set Lume url on my import map just for LSP.
On the real situation of building the site, the lume version specified through import map is better to be same as the running lume.

@oscarotero
Copy link
Member

I assume a user may want to override the default import map of Lume, so the user provided configuration should replace the default one. Why do you want to define a different import map for Lume, but at the same time, Lume override it?

@aiotter
Copy link
Contributor Author

aiotter commented Jan 19, 2022

It's for LSP. I have to tell my code editor what that import "lume" means in my code.
And I often switch the Lume path when I am modifying Lume itself, so it's often the case that Lume path specified on deno run command is different from the one on import map.

Do you think user want to override the lume path? Isn't it the partial monkey patch, which would be done by scope section of the import map?

@oscarotero
Copy link
Member

Mmm, I see.
Lume has the lume init --only=vscode to configure vscode for that. Maybe we can improve this command and allow to assign a different import map file?

@aiotter
Copy link
Contributor Author

aiotter commented Jan 19, 2022

Hmm, I don't think it is very good idea.
I'm now using vim-lsp and vim-lsp-settings for LSP.
You need to specify ${project_root}/.vim-lsp-settings/settings.json like following if you want to set import map other than ${project_root}/import_map.json.

{
  "deno": {
    "initialization_options": {
      "enable": true,
      "lint": true,
      "unstable": true,
      "importMap": "alternative_import_map.json"
    }
  }
}

This kind of settings are different by LSP provider. I don't think it's possible to have a lot of options to lume init for every LSP provider.

Rather than that, it's nice to have an option for switching this behaviour to override the default or not, like lume --weak-import-map=import_map.json.
(What a terrible naming... It's midnight here in Japan and I'm almost falling asleep)

@oscarotero
Copy link
Member

Mmm, I understand.

Maybe we can have a lume init --import-map (or something like that) that only create/update an import map, so you can use it to customize any LSP provider.

@aiotter
Copy link
Contributor Author

aiotter commented Jan 20, 2022

But in that case I have to maintain two different import map. One for LSP, the other for building site. It's against DRY.

If deno LSP can read two different import map, your suggestion seems the best. But it's not the case, so...

@aiotter
Copy link
Contributor Author

aiotter commented Jan 20, 2022

How about creating import map inside the _config.ts?
You can import it in ci.ts like import {importMap} from "./_config.ts". In this case we can build import map dynamically at runtime before building a site. You may export the default import map from ci.ts, which enables users to import it in _config.ts to merge with their own import map for LSP.

Or more simply, import it from ./_importMap.ts.

@oscarotero
Copy link
Member

oscarotero commented Jan 20, 2022

But in that case I have to maintain two different import map. One for LSP, the other for building site. It's against DRY.

Mmm, not sure to understand. I mean creating just one import map that can be used by everything. For example, you can have this import map:

{
    "imports": {
        "std/": "https://deno.land/std@0.121.0/"
    }
}

and after running lume import-map ./import_map.json, the import map is updated with the lume imports, but keeping the other existing values:

{
    "imports": {
        "std/": "https://deno.land/std@0.121.0/",
        "lume": "https://deno.land/x/lume@v1.4.3/mod.ts",
        "lume/": "https://deno.land/x/lume@v1.4.3/",
        "https://deno.land/x/lume/": "https://deno.land/x/lume@v1.4.3/"
    }
}

So, this file can be used by the LSP, VSCode config, etc.

@aiotter
Copy link
Contributor Author

aiotter commented Jan 21, 2022

Ah, I got what you mean. That import-map sub-command is an updater of import map. It will be called after lume upgrade so that import map would match the latest lume version. Nice.

Additionally, how about warning if the version of Lume is different from that of specified at import map?
Just like notification of available update:

lume/cli.ts

Lines 152 to 160 in a6a8a91

if (info) {
console.log("----------------------------------------");
console.log(
`Update available ${dim(info.current)}${green(info.latest)}`,
);
console.log(`Run ${cyan(info.command)} to update`);
console.log("----------------------------------------");
}
}

@oscarotero
Copy link
Member

Yes, good point.

@oscarotero
Copy link
Member

The latest development version of lume (lume upgrade --dev) includes the lume import-map command to generate/update import maps files.

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

No branches or pull requests

2 participants