-
Notifications
You must be signed in to change notification settings - Fork 122
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: add support for 3rd party resolver #188
Conversation
Signed-off-by: Jordan Hall <jordan@libertyware.co.uk>
a940af1
to
4acc43f
Compare
@@ -33,7 +33,7 @@ function pagesPlugin(userOptions: UserOptions = {}): Plugin { | |||
ctx.setupViteServer(server) | |||
}, | |||
resolveId(id) { | |||
if (MODULE_IDS.includes(id)) | |||
if (id === userOptions.moduleId || MODULE_IDS.includes(id)) |
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.
This will not works, because it will resolved to the same virtual id.
For multiple instance, we need different virtual id.
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 was thinking you can just use different ones for the plugin:
Pages({
// load some vue files
}),
Pages({
react: true,
moduleId: '~react-pages-billing'
}),
Pages({
react: true,
moduleId: '~react-pages-crm'
}),
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 think we need some new interface for resolver, because you still need to handle the other parts like stringify dynamic import for different frameworks.
Looking at the work from Angular. It seems to work ok, However, I do agree a new interface would be better as I notice Vue is different from the others. I'll work on the new interface and update |
Signed-off-by: Jordan Hall <jordan@libertyware.co.uk>
I was thinking Maybe the resolver doesn't need a different interface it makes sense as a simple resolver. My thinking is to add a Page controller (taken from page context) In this regard people can extend from the current PageContext and override the current functionality. This way people don't have to implement everything unless it's required. Option 1:
Option 2: make the Resolver class have the IPageController
In option two, I'll create a base class which is an abstraction of PageContext. We remove the functions intro another class which then allows people to extend the original class and overwrite the functionality |
Signed-off-by: Jordan Hall <jordan@libertyware.co.uk>
@hannoeru I've made Page Context changeable. I'm not sure if this should just be the Resolver or to allow the two to be set independently |
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 for this PR but I think this is too complex, we can just change resolver's interface to add more hooks like rollup's plugin, that we can use the same resolver interface for internal resolver and 3rd party resolver, don't need to change entire page context structure.
interface PageResolver {
resolvePages()
onClientGenerate()
onPageAdded()
onPageRemoved()
onPageChanged()
stringifyComponent()
}
This PR closes Custom Resolvers #156.
Created this so someone can create an Angular resolver now we have Angular working with vite