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

Add __STORYBOOK_CLIENT_API__ for external tools #3058

Merged
merged 2 commits into from
Feb 23, 2018

Conversation

tmeasday
Copy link
Member

So tools like Chromatic and others that interact with the client API can work with storybooks that haven't injected any code.

I would love to refactor this code from preview into core but I will leave to a later PR.

How to test

Check that __STORYBOOK_CLIENT_API__ is defined in each example app.

@tmeasday tmeasday requested a review from a team February 23, 2018 01:02
@joscha joscha changed the title Add __STORYBOOK_CLIENT_API for external tools Add __STORYBOOK_CLIENT_API__ for external tools Feb 23, 2018
@joscha
Copy link
Member

joscha commented Feb 23, 2018

cc @timhaines

@codecov
Copy link

codecov bot commented Feb 23, 2018

Codecov Report

Merging #3058 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3058      +/-   ##
==========================================
- Coverage   36.87%   36.86%   -0.02%     
==========================================
  Files         417      417              
  Lines        9849     9853       +4     
  Branches      830      839       +9     
==========================================
  Hits         3632     3632              
+ Misses       5750     5736      -14     
- Partials      467      485      +18
Impacted Files Coverage Δ
app/polymer/src/client/preview/index.js 0% <0%> (ø) ⬆️
app/angular/src/client/preview/index.js 0% <0%> (ø) ⬆️
app/react/src/client/preview/index.js 0% <0%> (ø) ⬆️
app/vue/src/client/preview/index.js 0% <0%> (ø) ⬆️
...-native/src/preview/components/OnDeviceUI/index.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/search_box.js 23.52% <0%> (ø) ⬆️
lib/ui/src/modules/api/configs/init_api.js 40.42% <0%> (ø) ⬆️
addons/a11y/src/components/Report/Info.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/addon_panel.js 23.52% <0%> (ø) ⬆️
addons/info/src/components/types/OneOf.js 61.11% <0%> (ø) ⬆️
... and 57 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 4edba5e...c8307fa. Read the comment docs.

@tmeasday
Copy link
Member Author

Yes, interested to know if this helps Percy and if there is more that would be needed at your end @timhaines

@ndelangen ndelangen merged commit b014b25 into master Feb 23, 2018
@ndelangen ndelangen deleted the tmeasday/attach-store-to-window branch February 23, 2018 06:46
const clientApi = new ClientApi(context);
export const { storiesOf, setAddon, addDecorator, clearDecorators, getStorybook } = clientApi;
// Provide access to external scripts
window.__STORYBOOK_CLIENT_API__ = clientApi;
Copy link
Member

Choose a reason for hiding this comment

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

@tmeasday Is there any particular reason to do that only inside isBrowser branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess window is not defined in the other context (i.e. jest). Are you thinking we could add it to global.__STORYBOOK_CLIENT_API__?

Copy link
Member

@Hypnosphi Hypnosphi Mar 20, 2018

Choose a reason for hiding this comment

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

I would like to use it from jsdom, where window is actually defined and is not the same as global. My usecase is to get stories tree from node.js script

Copy link
Member Author

Choose a reason for hiding this comment

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

This works in JSDOM. Have you tested it? Storybook doesn't know the difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I literally tested this yesterday, that's why I feel quite confident about this ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I did 😊

When was that line added? That doesn't seem helpful to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

JSDOM is trying to pretend to be a browser, why would we want to treat it differently?

Copy link
Member

@Hypnosphi Hypnosphi Mar 20, 2018

Choose a reason for hiding this comment

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

Probably because it lacks non-dom api like postmessage that is used in channels

Cc @ndelangen
#2542 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, fair enough.

Well I think it is fine for this particular check just be something if (typeof window !== 'undefined') but I suspect there will be other use cases for JSDOM where having isBrowser === true might be helpful. If it breaks however I guess we leave it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also any thoughts on #3058 (comment)

@tmeasday
Copy link
Member Author

Would it perhaps also be useful to expose the channel, perhaps on __STORYBOOK_CHANNEL__? That way scripts could also fire events to trigger things like stories rendering.

@Hypnosphi
Copy link
Member

Feel free to add as long as you're going to use it =)

@tmeasday
Copy link
Member Author

@Hypnosphi => #3243

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants