(Vite 5): breaking `import.meta.glob` API needs corrections

MajedMuhammad commented 8 months ago

Describe the bug

There should be tight alignments while breaking the import.meta.glob API. This bug report discusses the second parameter, the options argument.

as should be removed from the types/interfaces while it has been removed from the docs.
(Overload): The type inferring relies on two options: eager and as. This is impossible while nothing is called as in the new interface! Noting that the inferring can now (if there won't be generics) rely on eager, query, AND import.
In the build process or the parsing glob options, there is a wrap of as by query. This is not clear in the docs. Also, it is worth noting that the question mark ? prefix became mandatory for raw, url, and init.

Proposed Actions

Decide whether to keep the as option.
1. Keeping it?
  1. Note this option in the docs.
2. Removing it totally?
  1. Modify the options interface to be not accepting as anymore (static-type).
  2. Modify the parsing process (to avoid errors in the build process).

Either way, there should be:

A note in the docs that the question mark ? is mandatory now for the built-in supported types (mentioned above), support both with ? or without, or support only without ? and the mark would be injected automatically if the process needs it.
Alter the type inferring to consider also the import option, if there is no intent to introduce generics here.

Reproduction

generic

Steps to reproduce

No response

System Info

Vite 5

Used Package Manager

pnpm

Logs

No response

Validations

[X] Follow our Code of Conduct
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to vuejs/core instead.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
[X] The provided reproduction is a minimal reproducible example of the bug.

MajedMuhammad commented 8 months ago

(Critical Bug): It is worth noting that there is a difference between the import.meta.glob behaviour in dev and build with postfix url.

In dev mode, it is working as expected. In the preview mode, it is loading it as raw!

MajedMuhammad commented 7 months ago

To keep a consistent track, I found the related PR #14420.

MajedMuhammad commented 7 months ago

It comes to my attention that:

(Critical Bug): It is worth noting that there is a difference between the import.meta.glob behaviour in dev and build with postfix url.

In dev mode, it is working as expected. In the preview mode, it is loading it as raw!

is caused not by the import.meta.glob but by the inlining process.

It seems we have a new feature that assets may be inlined, but this causes the build to produce incompatible assets especially when the code references an asset's URL as the case I mentioned. At that point, the produced object (during build) by import.meta.glob will look like:

{
    "path": "raw"
}

instead of:

{
    "path": "encoded_URL"
}

As a workaround, disabling the inlining process by setting assetsInlineLimit: 0 in Vite configs (build).

MajedMuhammad commented 7 months ago

as should be removed from the types/interfaces while it has been removed from the docs.

For this, in the latest version, I see it has been marked as deprecated. LGFM.

However, it is still used for inferring! How do rely on something deprecated? The next reply discusses the inferring type for return.

MajedMuhammad commented 7 months ago

(Overload): The type inferring relies on two options: eager and as. This is impossible while nothing is called as in the new interface! Noting that the inferring can now (if there won't be generics) rely on eager, query, AND import.

This one is still not resolved yet. It seems that is defined as unknown type for return, which should be something like:

type GlobReturnPrototype =
    Query == "raw || url" ? Record<string, string>
    Query == undefined && fileExtension == "ts || js" ? Record<string, M>; M = Func || Object || Var.

import == 'default' ? GlobReturnPrototype
: () => GlobReturnPrototype

It is just a pseudo-code for clarification.

MajedMuhammad commented 7 months ago

In the build process or the parsing glob options, there is a wrap of as by query. This is not clear in the docs. Also, it is worth noting that the question mark ? prefix became mandatory for raw, url, and init.

As to make a stable base for long-term maintenance (even if it is minor), IMO, all queries should be known types such as url, without adding ?. If it is necessary for fs tool (if one is used) to have the query in such style (? first, & between) then this can be done programmatically, not developer/user matter, and to not accept ? anymore.

MajedMuhammad commented 7 months ago

Remaining one bug related here: The inlining process should exclude those assets that are imported as URLs (whether a dynamic import or a glob import).

bluwy commented 7 months ago

Can you summarize the issue here? We are not removing the as from the types yet, only marking as @deprecated because it still works today if you use it, but will emit a warning. Also, you don't need to prefix with ? if you use the query option.

MajedMuhammad commented 7 months ago

Sure. Here is the summary:

as is used for inferring the type, while itself is deprecated. When ts dev avoids deprecated as and uses query instead, will get unknown as the return type (type bug).
Avoiding ? so ts dev will have to choose one of the acceptable values (e.g., type query = 'raw' | 'url' | 'worker' ...etc | Record<string, string | query> so dev can (with help of code editor) know which values are acceptable easily, and then inject ? internally if needed (no need to share ? with dev, vite itself should handle it under the hood.) (API design or type bug).
Here we have overlapped issue with the inlining assets feature, by default that feature (as I expect) is inlining assets for a specific range of sizes, the issue lies in sometimes we require some assets as url using dynamic import or (using OUR CASE here) glob import, we surprisingly get those assets as raw instead of url after the build, which may crashes or does something inadvertently the app. (Might be an inlining bug and should be separate) (major bug). * After observation, the assets basically after build are inlined and do not exist in the /asset directory.

That's all. Thanks for your patience and for reading all my comments.

vitejs / vite