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

feat: improve dynamic import variable failure error message #16519

Merged
merged 3 commits into from May 2, 2024

Conversation

btea
Copy link
Collaborator

@btea btea commented Apr 24, 2024

Description

As shown in the figure below, when the dynamic import component fails, the error message Unknown variable dynamic import: ./components/a/b/test.vue will appear.

But judging from the code structure, there is no problem with the path. I think this information may not be enough for people to immediately and clearly determine where the problem occurs, so outputting pattern information may be able to explain the problem more clearly and help users locate the problem more quickly.

image

Copy link

stackblitz bot commented Apr 24, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@bluwy
Copy link
Member

bluwy commented Apr 29, 2024

I think in general, we want to hide import.meta.glob as an implementation detail of dynamic imports only, so exposing it in the error message could be more confusing?

IIUC the issue is that some may forgot that ${var} cannot contain slashes. Could we detect cases like this in the error message instead? (maybe checking number of /). Or I suppose linking directly to https://vitejs.dev/guide/features.html#dynamic-import could be fine too. We need to also make sure to not make the helper code too large as it's injected to the runtime.

@btea
Copy link
Collaborator Author

btea commented Apr 29, 2024

It makes sense, I just noticed that the document states Note that variables only represent file names one level deep.

Maybe we can add the variable inclusion level depth may be more than one, please refer to https://vitejs.dev/guide/features.html#dynamic-import in the error message.

@bluwy
Copy link
Member

bluwy commented Apr 29, 2024

I think maybe directly appending Note that variables only represent file names one level deep should also be fine. It would be nice to optionally show it if we know it happens, but if not it also works for me.

The link currently doesn't have a lot of information, so might not be worth adding it for now.

@btea btea changed the title chore: dynamic import variable failure error message display pattern chore: update dynamic import variable failure error message Apr 30, 2024
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work locally for me 👍 Curious to hear what others think of this extra code added before merging it.

@bluwy bluwy changed the title chore: update dynamic import variable failure error message feat: improve dynamic import variable failure error message Apr 30, 2024
@bluwy bluwy added the p2-nice-to-have Not breaking anything but nice to have (priority) label Apr 30, 2024
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with the added param. Another option is to add a link that is always there and explain more in the docs. Or reword it as a tip but it may confuse more than it helps.

@btea
Copy link
Collaborator Author

btea commented Apr 30, 2024

Maybe we can add a detailed explanation chapter about this situation to the troubleshooting page in the future.

@bluwy bluwy merged commit f8feeea into vitejs:main May 2, 2024
12 checks passed
@btea btea deleted the feat/improve-dynamic-import-var-message branch May 2, 2024 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants