-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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(warn): warn defineAsyncComponent usage in routes #682
Conversation
src/navigationGuards.ts
Outdated
@@ -294,6 +301,10 @@ export function extractComponentsGuards( | |||
const resolvedComponent = isESModule(resolved) | |||
? resolved.default | |||
: resolved | |||
// __asyncLoader is added by defineAsyncCompoent | |||
if (__DEV__ && '__asyncLoader' in resolvedComponent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? What does it change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@posva
Async component can be used inside dynamic imports route components.
This will mark the async component is used inside dynamic imports route component or not.
I don't know is it the right way, or do like
if (__DEV__ && '__asyncLoader' in resolvedComponent) {
resolvedComponent.__lazyResolved = true;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this accounting for () => defineAsyncComponent(...)
? If it is, it should warn instead and it should also have a test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also accounting for any function return Promise<AsyncComponent>
Is this necessary?
and if component not return Promise. it will warn like this first at line 247.
Component "default" in record with path "/foo" is a function that does not return a Promise. If you were passing a functional component, make sure to add a "displayName" to the component. This will break in production if not fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just kept the same unresolved points!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just kept the same unresolved points!
Thank you for reviewing !
Those unresolved potints are all about the lazy load async component.
It's hard to distinguish between async component and dynamic imports async component after cached.
Because the the following code cache async component.
componentPromise.then(resolved => {
const resolvedComponent = isESModule(resolved)
? resolved.default
: resolved
// replace the function with the resolved component
record.components[name] = resolvedComponent
}
compont will be async componet
instead of Promise<async component>
when navigate to this route next time .
So I marked the resolved async componet.
if (__DEV__ && '__asyncLoader' in resolvedComponent) {
resolvedComponent.__resolvedAsyncComponent = true
}
and check the mark when async componet.
else if (
(rawComponent as any).__asyncLoader &&
!(rawComponent as any).__resolvedAsyncComponent // check dynamic imports
) {
// make it component promise , it will marked by code above, and warn only once when navigated first time.
let asyncComponent = rawComponent
rawComponent = () => Promise.resolve(asyncComponent)
warn(.... )
}
}
the next time navigate to this component, we can know it is dynamic imports or not.
Sorry about my poor English. hope you understand what I said.🙏
src/navigationGuards.ts
Outdated
@@ -294,6 +301,10 @@ export function extractComponentsGuards( | |||
const resolvedComponent = isESModule(resolved) | |||
? resolved.default | |||
: resolved | |||
// __asyncLoader is added by defineAsyncCompoent | |||
if (__DEV__ && '__asyncLoader' in resolvedComponent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@posva
Async component can be used inside dynamic imports route components.
This will mark the async component is used inside dynamic imports route component or not.
I don't know is it the right way, or do like
if (__DEV__ && '__asyncLoader' in resolvedComponent) {
resolvedComponent.__lazyResolved = true;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think I understand what you said. I realized I wrote one of my comments at the wrong line. Let me know if the review isn't clear
src/navigationGuards.ts
Outdated
@@ -294,6 +301,10 @@ export function extractComponentsGuards( | |||
const resolvedComponent = isESModule(resolved) | |||
? resolved.default | |||
: resolved | |||
// __asyncLoader is added by defineAsyncCompoent | |||
if (__DEV__ && '__asyncLoader' in resolvedComponent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this accounting for () => defineAsyncComponent(...)
? If it is, it should warn instead and it should also have a test
I remove the accounting for because it will warn and remove all warn for function return Promise,like it will warn when navigate to this component next time .Beacuse next time,the component here is just |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I couldn't push some final changes (you should check the allow edits by maintainers) so I will do that locally
Thank you for your patient review! I'm really excited about this PR was merged! |
Close #627