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

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

Open
7 tasks done
MajedMuhammad opened this issue Feb 17, 2024 · 9 comments
Open
7 tasks done

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

MajedMuhammad opened this issue Feb 17, 2024 · 9 comments
Labels
cannot reproduce The bug cannot be reproduced pending triage

Comments

@MajedMuhammad
Copy link
Contributor

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.

  1. as should be removed from the types/interfaces while it has been removed from the docs.
  2. (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.
  3. 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:

  1. 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.
  2. 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

@MajedMuhammad
Copy link
Contributor Author

MajedMuhammad commented Feb 17, 2024

(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
Copy link
Contributor Author

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

@MajedMuhammad
Copy link
Contributor Author

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
Copy link
Contributor Author

MajedMuhammad commented Feb 19, 2024

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
Copy link
Contributor Author

(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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

bluwy commented Feb 21, 2024

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.

@bluwy bluwy added the cannot reproduce The bug cannot be reproduced label Feb 21, 2024
@MajedMuhammad
Copy link
Contributor Author

MajedMuhammad commented Feb 21, 2024

Sure.
Here is the summary:

  1. 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).
  2. 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).
  3. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cannot reproduce The bug cannot be reproduced pending triage
Projects
None yet
Development

No branches or pull requests

2 participants