-
-
Notifications
You must be signed in to change notification settings - Fork 183
feat(vue)!: separate include and exclude from api.options and add filter
#582
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
Conversation
commit: |
packages/plugin-vue/src/index.ts
Outdated
| filter: { | ||
| // FIXME: should include other files if filter is updated | ||
| id: { | ||
| include: /[?&]vue\b|\.vue$/, | ||
| exclude: /[?&](?:raw|url)\b/, | ||
| }, | ||
| }, |
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.
To have this part of change, we need to introduce a breaking change.
The filter has to have a same value after configResolved hook is called (because the value is passed to rolldown and cannot be modified), but currently options.value.include / options.value.exclude can be changed in any time.
I can revert this part for now so we can merge the other parts and make a separate PR if that's better.
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.
The same applies to other cases where options.include/.exclude exists (e.g. in https://github.com/TheAlexLichter/unplugin-auto-import/blob/9c06cb618ca5fcabfe4ec1729e24eea2d87a48be/src/core/unplugin.ts#L20-L22)?
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 it doesn't apply to those cases.
In this plugin-vue's case, options.value is exposed via api, which is meant to be used to update the option.value. So I think the contract here is that setting api.options will update the option.
On the other hand, in that unplugin-auto-import case, I think people won't expect options.include to be modified after the plugin is instantiated. For example, the code below would do that, but I guess people won't expect this to work unless explicitly documented.
const options = { include: [] }
setTimeout(() => {
options.include.push('something') // or options.include = ['something-else']
}, 1000)
const plugin = autoImport(options)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.
@sxzz What are your thoughts on this? (since you introduced this feature)
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.
Vue Macros is using this API to modify some options like template.compilerOptions.
Can we make api.include/exclude as a getter/setter, linking to transform.filter?
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.
Can we make
api.include/excludeas a getter/setter, linking totransform.filter?
Do you mean something like below?
let filter = {}
const p = {
api: { get include() { return filter.include }, set include(v){ filter.include = v } },
transform: { filter, handler() { /* omit */ } }
}Is it to make it clear that include/exclude cannot be updated after configResolved hook is called?
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.
Yes, we can add a jsdoc
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.
@sxzz I've updated the PR 👍
api.options and add filter
|
@sxzz @haoqunjiang @edison1105 Do you have any thoughts on the next major release? |
|
LGTM~ |
|
I prefer to release a beta major version. |
|
OK. I'll merge this and cut a beta. |
Description
Adds
filterto plugin-vue so that it makes the plugin performant when used in rolldown-vite.Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).