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

fix(plugin-vue): respect __VUE_PROD_DEVTOOLS__ setting #4984

Merged
merged 7 commits into from Mar 24, 2022

Conversation

oliverzy
Copy link
Contributor

@oliverzy oliverzy commented Sep 19, 2021

Description

If user defines VUE_PROD_DEVTOOLS to true, plugin-vue should respect this to make sure the generated code can be compatible with Vue devtools (inlineTemplate and filename)

Additional context

script setup with inlineTemplate as true is not compatible with Vue devtools since it doesn't expose any bindings.
Vue devtools relies on filename to infer the component name for the component using script setup.

What is the purpose of this pull request?

  • Bug fix

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link
Member

@sodatea sodatea left a comment

Choose a reason for hiding this comment

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

Please don't commit style-only changes on files irrelevant to the PR.

# Conflicts:
#	packages/playground/ssr-vue/example-external-component/ExampleExternalComponent.vue
@oliverzy
Copy link
Contributor Author

OK, it's done.

@@ -58,6 +58,7 @@ export async function transformMain(
// inlined template cannot be individually hot updated.
const useInlineTemplate =
!devServer &&
!devToolsEnabled &&
Copy link
Member

@sodatea sodatea Sep 27, 2021

Choose a reason for hiding this comment

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

Seems exposing __file is enough for the devtool to display the component name, why do you want to disable inlineTemplate?

Copy link
Contributor Author

@oliverzy oliverzy Sep 27, 2021

Choose a reason for hiding this comment

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

Because for component with script setup, inlineTemplate as true doesn't expose any setup binding. Vue devtools relies on this to display component setup binding correctly.

Copy link
Member

@sodatea sodatea Sep 27, 2021

Choose a reason for hiding this comment

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

Got it.
I have discussed this with Evan today.
As the inlineTemplate option has an impact on the app performance, we believe that it shouldn't be disabled.
We need to find another way to fix this issue. Maybe in another package.

@@ -108,7 +109,7 @@ export async function transformMain(
if (hasScoped) {
attachedProps.push([`__scopeId`, JSON.stringify(`data-v-${descriptor.id}`)])
}
if (devServer && !isProduction) {
if (devToolsEnabled || (devServer && !isProduction)) {
// expose filename during serve for devtools to pickup
attachedProps.push([`__file`, JSON.stringify(filename)])
Copy link
Member

Choose a reason for hiding this comment

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

Due to safety concerns, the full path should not be used in production. Please use path.basename(filename).
The development bundle should still use the full path so that users can open the file directly from devtools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

Choose a reason for hiding this comment

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

Hi, I've just started using @vitejs/plugin-vue2 in my rollup project, and was surprised to see the full filepath to my modules included in the generated output (as __component__.options.__file), and have not found a way to disable this. Can you advise on how to remove these filepaths?

Copy link
Member

@sodatea sodatea left a comment

Choose a reason for hiding this comment

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

Please remove the changes related to inlineTemplate.
We need to open an issue in the vue-next repo to provide the ability required by Vue DevTools, rather than work around the issue in the Vite plugin.

The __file fix looks good to me.

@flxsource
Copy link

Please remove the changes related to inlineTemplate. We need to open an issue in the vue-next repo to provide the ability required by Vue DevTools, rather than work around the issue in the Vite plugin.

The __file fix looks good to me.

Has the vue-next (now vuejs/core?) issue been raised? I haven't been able to find anything relevant, but want to make sure I'm not duplicating the issue.

@sodatea
Copy link
Member

sodatea commented Mar 1, 2022

Raised in vuejs/core#5496

@patak-dev
Copy link
Member

@sodatea is this one ready to merge?

@sodatea
Copy link
Member

sodatea commented Mar 24, 2022

@patak-dev Yeah, I forgot to approve it after pushing…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants