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

Update global hook for Vue Devtools #1376

Merged
merged 4 commits into from
Jul 23, 2017

Conversation

matt-oconnell
Copy link
Contributor

@matt-oconnell matt-oconnell commented Jun 28, 2017

Issue:

What I did

Right now, Vue Devtools cannot identify Vue root nodes within iframes. We can link a global hook from the parent window to the iframe window and devtools will successfully recognize that the page includes a Vue application. This causes the devtools add the tab to browser debugging windows.

Secondly, adding a __VUE_DEVTOOLS_CONTEXT__ variable and defining it as the iframe document, allows the devtools to properly walk the context and identify components. To work, this requires a change to Vue Devtools as well.

How to test

Run storybook and attempt to use Vue Devtools. Without the corresponding PR to add __VUE_DEVTOOLS_CONTEXT__, you should still see Vue Devtools but Components will not appear correctly

I'm going to submit a PR to devtools which I'll reference here as well.

@codecov
Copy link

codecov bot commented Jun 28, 2017

Codecov Report

Merging #1376 into release/3.2 will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.2    #1376      +/-   ##
===============================================
+ Coverage        20.44%   20.45%   +0.01%     
===============================================
  Files              241      236       -5     
  Lines             5215     5143      -72     
  Branches           643      627      -16     
===============================================
- Hits              1066     1052      -14     
+ Misses            3662     3617      -45     
+ Partials           487      474      -13
Impacted Files Coverage Δ
app/vue/src/server/iframe.html.js 0% <ø> (ø) ⬆️
addons/knobs/src/react/WrapStory.js 11.11% <0%> (-0.89%) ⬇️
lib/ui/src/modules/ui/routes.js 0% <0%> (ø) ⬆️
app/vue/src/client/preview/client_api.js 86.84% <0%> (ø) ⬆️
lib/ui/example/client/provider.js 0% <0%> (ø) ⬆️
...react-native/src/manager/components/PreviewHelp.js 0% <0%> (ø) ⬆️
addons/notes/src/react.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/layout/usplit.js 38.7% <0%> (ø) ⬆️
...codemod/src/transforms/update-organisation-name.js 40.62% <0%> (ø) ⬆️
addons/knobs/src/components/types/Boolean.js 11.62% <0%> (ø) ⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b30336...ecd054d. Read the comment docs.

@shilman shilman changed the base branch from add-app-vue to release/3.2 July 6, 2017 07:55
@ndelangen ndelangen added maintenance User-facing maintenance tasks vue labels Jul 6, 2017
@shilman shilman removed their assignment Jul 7, 2017
@shilman
Copy link
Member

shilman commented Jul 15, 2017

@ndelangen were you ever able to verify that this works?

@matt-oconnell
Copy link
Contributor Author

@shilman this won't work fully until there is a change in Vue Devtools. reference: vuejs/devtools#361

cc: @posva who I believe is looking into a more in-depth fix for the Devtools which will allow them to identify apps within iframes. @posva Should we still keep the global __VUE_DEVTOOLS_GLOBAL_HOOK__ in this PR?

@posva
Copy link

posva commented Jul 16, 2017

I think we can keep it and support the global variable in a slightly different way in the future. I still need more time to work on this, I have only hacked on 1 afternoon or so 😅

Is the idea to allow auto focus of the devtools on the iframe while in storybook?

@matt-oconnell
Copy link
Contributor Author

@posva No worries, just thought I'd get your input. Maybe if the global variable is provided, use that context by default? Otherwise, traverse the DOM (including iframes) until a root Vue instance is detected and use that?

@posva
Copy link

posva commented Jul 17, 2017

Yeah, but we can do that on Vue devtools end 🙂

@ndelangen
Copy link
Member

@posva @matt-oconnell should we merge this then?

If it's going to work as-is in the future, there's no harm in merging?
How do you feel about this @shilman ?

@matt-oconnell
Copy link
Contributor Author

👍

@@ -56,7 +56,8 @@ export default function({ assets, publicPath, headHtml }) {
<meta name="viewport" content="width=device-width, initial-scale=1">
<script>
if (window.parent !== window) {
window.__REACT_DEVTOOLS_GLOBAL_HOOK__ = window.parent.__REACT_DEVTOOLS_GLOBAL_HOOK__;
window.__VUE_DEVTOOLS_GLOBAL_HOOK__ = window.parent.__VUE_DEVTOOLS_GLOBAL_HOOK__;
window.parent.__VUE_DEVTOOLS_CONTEXT__ = window.document;
Copy link

Choose a reason for hiding this comment

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

@matt-oconnell I don't know how storybook works, why do we need this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't unless we want to ensure Devtools looks directly at this context. It would possibly speed up start up time a bit. I can remove it if you want me to

Copy link

@posva posva Jul 18, 2017

Choose a reason for hiding this comment

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

no, it looks fine like this 🙂

Thanks for the follow up!

@shilman
Copy link
Member

shilman commented Jul 22, 2017

@matt-oconnell are we still waiting on changes to vue devtools? LMK! we're planning to do the 3.2 release soon and it would be great to get this in if that's realistic.

@posva
Copy link

posva commented Jul 22, 2017

Things look fine for me. We'll be able to update the devtools later, don't worry about that 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks vue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants