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

Support webpack4 modules format #3576

Merged
merged 1 commit into from
May 17, 2018
Merged

Conversation

theKashey
Copy link
Contributor

Issue: storybook 4.0.0 does not pass filename to story context, as long module.filename is not defined.

What I did

I've changed module.filename to module.name, and added one test, to test storyOf function agains real module variable. (double checked in browser)

How to test

As usual. But you might need run storybook to test integration with webpack in real.

Not sure about keeping old way for accessing fileName, but, look like, it's obsolete.

@storybook-safe-bot
Copy link
Contributor

Fails
🚫

PR is not labeled with one of: ["cleanup","BREAKING CHANGE","feature request","bug","documentation","maintenance","dependencies:update","dependencies","other"]

Generated by 🚫 dangerJS

Copy link
Member

@igor-dv igor-dv left a comment

Choose a reason for hiding this comment

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

Initially, the fileName was added to make it available during this storyshots a.k.a. running in node.
Does it still happen (I assume that it does, since all the tests are passing)?

@theKashey
Copy link
Contributor Author

@igor-dv - there were no tests against "real" module variable.
Meanwhile nodejs does contain both filename and id. Webpack actually does not include any of them, only i + "adapter", resulting id as a getter.

@ndelangen
Copy link
Member

Thank you @theKashey that's pretty advanced for a first PR 🙇

What are potential negative side effects of this?

@theKashey
Copy link
Contributor Author

There is only one drawback - I could guarantee that this will work for old node.js versions, but could not say the same for the older webpack versions. But this is not the case.
But I am pretty sure that .id is the right way to access file name. I am confident about the change due to my contributions to dependency mocking libraries. And tests.
As this PR is not changing anything, just fixing the property name, and I need non-emptyfilename to continue with another PR.

@ndelangen ndelangen added configuration babel / webpack maintenance User-facing maintenance tasks labels May 17, 2018
@ndelangen
Copy link
Member

@igor-dv do you agree we can merge this?

@igor-dv
Copy link
Member

igor-dv commented May 17, 2018

Yeah. Does it make sense to mark it as breaking change?

@ndelangen
Copy link
Member

Yes, 4.0.0 material for sure.

@ndelangen ndelangen merged commit 9d4438b into storybookjs:master May 17, 2018
@ndelangen
Copy link
Member

Congrats on your first merged PR @theKashey

@theKashey
Copy link
Contributor Author

Will send more :)

@Hypnosphi
Copy link
Member

Released as 4.0.0-alpha.7

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

Successfully merging this pull request may close these issues.

5 participants