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

Proposal: add import maps to the config file #12800

Closed
kitsonk opened this issue Nov 17, 2021 · 9 comments · Fixed by #13739
Closed

Proposal: add import maps to the config file #12800

kitsonk opened this issue Nov 17, 2021 · 9 comments · Fixed by #13739
Assignees
Labels
feat new feature (which has been agreed to/accepted) suggestion suggestions for new features (yet to be agreed)

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Nov 17, 2021

Context

Deno currently supports a config file and Deno currently supports import maps. Deno does not currently support "combining" import maps and the config file. This leads to potentially complicated command line invocations.

In addition the feature of deno vendor is being added (see: #13346) as well as there is other tooling which automates the building of import maps within Deno.

As of Deno 1.18, the configuration file is automatically detected, that coupled with significant use of the community of import maps, it is a significantly better developer experience to be able to store the import map in the configuration file and be able to elide that on the command line, just as users can elide the --config file option now.

Edit: This proposal has been updated from its original form

Semantics

  • If a config file applies, and there is a top level key named "importMap", the value of the key will be interpreted as a path or URL using the config file as a base.
  • If the flag --import-map is supplied on the command line, the value will be used as it is today. If a config file applies and contains an import map, the user will be warned that the value on the command line argument is being ignored, unless the value of importMap and --import-map resolve to the same resource.

Originally the semantics included support for the top level "imports" and "scopes" keys in the configuration file. This has been removed from this proposal and can be revisited in the future.

Examples

{
  "compilerOptions": {
    "lib": [ "dom", "dom.iterable", "deno.ns" ]
  },
  "importMap": "./import_map.json"
}
{
  "compilerOptions": {
    "lib": [ "dom", "dom.iterable", "deno.ns" ]
  },
  "importMap": "https://example.com/import-map.json"
}

Considerations

  • The semantics around --import-map or the import map in a config file taking precedence is debatable to the author of the proposal and the author isn't tied to one or the other, but we should defined semantics, and warn the user if there is one or the other being ignored. This was discussed in the core team, and it was agreed that --import-map should take precedence over the configuration file
  • There maybe considerations related to this proposal with Proposal: add tasks to the config file #12764. For example, a task should be able to define its own import map. This was discussed in the core team, and agreement was that if Proposal: add tasks to the config file #12764 moves forward, it will need to address this consideration, as well as other related considerations
  • There have been concerns around using the top level keys "imports" and "scopes". The argument has been "what if import maps change and then we have conflicts". The author of the proposal considers these not specifically material of the chance of change to the import map. Import maps was specifically designed to allow for excess keys in them and for them to be ignored, and there is a significant usability upside to allow an existing import map to be used as a config file without any changes. Basically people using an import map would be able to switch to --config without any changes to their import map, and tooling that maintains an import map could easily be applied directly to a config file without any changes. The biggest "risk" is that compositing import maps is still an open issue in the import map standard, but it hasn't really received any traction. Even then, it is likely we would have to adjust our import map implementation to accomodate it, but it doesn't feel right to hold back from making Deno more usable while there is a non-forward moving risk in the standard that may or may not cause challenges. Edit: this has been removed from the proposal and can be revisited in the future
@stale
Copy link

stale bot commented Jan 17, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 17, 2022
@crowlKats crowlKats added suggestion suggestions for new features (yet to be agreed) and removed stale labels Jan 17, 2022
@castarco
Copy link

castarco commented Jan 22, 2022

Hi, I'm not sure if I completely understand the proposal. Would that, somehow, help to "chain" import maps related to "nested" imports?

I'm putting myself in the role of library maintainer, for a library that has other external dependencies and wants to keep compatibility with NodeJS as well.

@kitsonk
Copy link
Contributor Author

kitsonk commented Jan 22, 2022

Would that, somehow, help to "chain" import maps related to "nested" imports?

No, it would not. Currently import maps are not chainable.

@jespertheend
Copy link
Contributor

I think having --import-map taking precedence over the import map from the config file would make things a lot more flexible. That would allow you to use a different import maps if needed. If the one from the config file would always take precedence, the import map would be severely locked in place.

Say you have two .bat or two .sh files for example, both calling deno run but each using a different --import-map. With the config taking precedence, something like this would not be possible without either creating two separate configs, or worse, modifying the config file from within the shell script.

@lowlighter
Copy link
Contributor

I agree with @jespertheend
I'd say it makes more sense anyway that any explicitely given cli argument takes precedence over the config one, not necessarly restricted to --import-map only.

Apart from this nitpick I think the proposal is fine.
It'd be a nice QOL by saving some time rather than currently using deno run --import-map deno.json with imports/scopes in the deno.json (which btw outputs warnings about excess keys unless --quiet is specified)

I hope it'll land in the near feature

@kitsonk kitsonk self-assigned this Feb 8, 2022
@kitsonk kitsonk added the feat new feature (which has been agreed to/accepted) label Feb 8, 2022
@dsherret
Copy link
Member

dsherret commented Feb 9, 2022

For import maps in deno.json, makes sense to only reference an import map file for now.

In the future, for an embedded import map in deno.json, I think it can be viewed the same way as an import map found in an html file under a <script type="importmap"> tag. Also, I don't think we should make "imports" and "scopes" top level and it would be under importMap to prevent future import map keys conflicting with ours. This will also prevent someone from using it as an import map in other tools (they should definitely be extracting it out to another file in that case).

@kidonng
Copy link
Contributor

kidonng commented Aug 27, 2022

It seems this can be now achieved by referencing the config file itself as the import map:

{
	"importMap": "deno.json",
	"imports": {
		"foo": "bar"
 	}
}

The only downside seems to be this:

import map diagnostics:
  - Invalid top-level key "importMap". Only "imports" and "scopes" can be present.

@dsherret
Copy link
Member

dsherret commented Aug 27, 2022

@kidonng that is a regression that we accidentally allow unsupported keys in deno.json. I'm going to open a fix to disallow that again. Edit: actually, looks like this was always supposed to be allowed.

@kidonng
Copy link
Contributor

kidonng commented Aug 28, 2022

There are already precedences like Velociraptor using deno.json. I will feel sorry but respect the decisions if you are going to have it to yourself.

That said, I don't think taking away the ability to do this for no apparent benefit is worth it. If Deno later uses imports and scopes for something different (which is unlikely, at least in the near future), it's only going to cause troubles for a minority of people employing the trick.

There are already warnings against this approach (albeit it's from import map instead of config diagnostics), if people accept the risks, let them.

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

Successfully merging a pull request may close this issue.

7 participants