-
-
Notifications
You must be signed in to change notification settings - Fork 172
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/exclude
as 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
filter
to 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
).