feat(glob-import): deprecate as option

[bluwy]

Description

Deprecate the glob as option in favour of query. In practice they do the same thing, but query is more flexible and reuses the existing knowledge of ?url, ?raw, ?worker queries.

Additional context

The gist of the changes are:

1. Pass logger instance around to log a warning.
2. Port special as error handling to query to keep feature parity.
3. Port glob options normalization, e.g. always normalize query as a string, to parseGlobOptions (new ParsedGeneralImportGlobOptions type)
4. Update existing repo as usage to query.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the PR Title Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[bolt-new-by-stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[sapphi-red]

I wonder if we should think whether we want to migrate to import attributes from queries.
I guess it's better to avoid a two step migration (deprecating as in favor of query and then deprecating query in favor of with).

[bluwy]

It's still unclear how import attributes will go forward, and I'm leaning towards how the spec enforces it before we start implementing our custom handling. For example, the spec mentions:

JavaScript implementations are encouraged to reject attributes and type values which are not implemented in their environment (rather than ignoring them). This is to allow for maximal flexibility in the design space in the future--in particular, it enables new import attributes to be defined which change the interpretation of a module, without breaking backwards-compatibility.

So what Vite has to do at the end of the day is convert it to queries again, which I think it's better to go to queries directly. query is also useful for Vite plugins so I don't see it deprecated any time soon.

[sapphi-red]

That makes sense 👍

[sapphi-red]

It would be great if we can infer the types from query property but I guess it's difficult to do it.

[bluwy]

Ah yeah I didn't notice as brings special default typings too. I guess they can still type it manually if needed, so it's not a huge issue.

It might be possible to infer it, but it very likely involves some dark magic and sacrifices.

[sapphi-red]

I feel it a bit too magic to change the default of import option based on query option. To me, as feels it might transform the output, but query only changes the query when imported.
That said, I think it'd be handy with this.

[bluwy]

That's true. I didn't think too much about it 🤔 I would be fine removing this and have the migration guide request adding import: 'default' too, especially given that this isn't typed anymore. Would you prefer that too?

[bluwy]

I removed the default import: 'default' configuration. This should make it more predictable.

[bluwy]

cc @antfu requesting your review since you authored glob imports before, and have a recent as: 'path' PR.

[antfu]

Regardless of spec, I tend to prefer the as option as it's more semantically readable. { as: 'raw' } vs { query: '?raw' }. Also, I am a bit worried about the type support, I guess something like { query: '?raw&foo=bar' } would lose the type.

I guess I am a bit conservative in general and I tend to keep things as-is if there is no much maintenance effort or absolute reason to drop something. Meanwhile, I understand you are trying to push a lot on removing things, huge respect here :). Thus I don't have a very strong opinion on this, we could move forward if the rest of the team agrees.

[patak-cat]

I'm kind of in the same boat as @antfu here. The query param isn't a great API, but as should be removed at one point. I'm fine waiting, although I think it may be a long time until the spec clarifies what we could use to replace queries in general. And once queries can also be replaced at the import level, then changing again for globs doesn't sound that scary as the user will be pushed to change queries in every import out there.

[bluwy]

The main issue for me is that I don't think import assertions/attributes is going to solve queries. They're not exactly the same thing, and queries tend to be more flexible, compact, and subjectively readable (e.g. vite-imagetools API).

There isn't a spec that covers it nicely yet, and I feel like that will be a lot of Vite majors away. But I'll keep this fair, and if there's another opposition to hold off for now I'll close the PR 👍

[bluwy]

I'll move this off 5.0 for now. It actually seems that this can be done non-breakingly but the deprecation would have to come in a minor if we do so.

[bluwy]

Note to self: There's a bit more perf improvements I found especially:

1. We don't need to call parseRequest, we can inline it and avoid its Object.entries call.
2. We can also extract the query string from id this way and pass it to here directly.

   vite/packages/vite/src/node/plugins/dynamicImportVars.ts

   Line 91 in bd26284

   query: rawQuery,

   (rawQuery is currently an object, could be a string instead) That way, we don't turn a string -> object -> string throughout the handling lifetime. (Strings are the source of truth)

I'll do this once the PR is merged so it's easier to review.

[sapphi-red]

Just one small nit, the rest LGTM 👍