vite
vite copied to clipboard
feat(glob-import): deprecate as option
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:
- Pass
logger
instance around to log a warning. - Port special
as
error handling toquery
to keep feature parity. - Port glob options normalization, e.g. always normalize
query
as astring
, toparseGlobOptions
(newParsedGeneralImportGlobOptions
type) - Update existing repo
as
usage toquery
.
What is the purpose of this pull request?
- [ ] Bug fix
- [x] New Feature
- [ ] Documentation update
- [ ] Other
Before submitting the PR, please make sure you do the following
- [x] Read the Contributing Guidelines.
- [x] Read the Pull Request Guidelines and follow the PR Title Convention.
- [x] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
- [x] 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.
Run & review this pull request in StackBlitz Codeflow.
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
).
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.
That makes sense 👍
cc @antfu requesting your review since you authored glob imports before, and have a recent as: 'path'
PR.
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.
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.
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 👍
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.
Note to self: There's a bit more perf improvements I found especially:
- We don't need to call
parseRequest
, we can inline it and avoid itsObject.entries
call. - We can also extract the query string from
id
this way and pass it to here directly. https://github.com/vitejs/vite/blob/bd26284a51d52ded70472ce92a359d884881fa43/packages/vite/src/node/plugins/dynamicImportVars.ts#L91 (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.