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

Addon-docs: Jest MDX transform for storyshots #8189

Merged
merged 16 commits into from
Oct 24, 2019
Merged

Addon-docs: Jest MDX transform for storyshots #8189

merged 16 commits into from
Oct 24, 2019

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Sep 25, 2019

Issue: #7223

This replaces #7330 since that PR no longer has a repository and cannot be contributed on anymore :(

What I did

Wrote a jest transform for mdx files.

How to test

add the following to your jest config:

{
  "transform": {
    "^.+\\.[tj]sx?$": "babel-jest",
    "^.+\\.mdx?$": "@storybook/addon-docs/jest-transform-mdx",
  }
}

@vercel
Copy link

vercel bot commented Sep 25, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/bs7viuaag
🌍 Preview: https://monorepo-git-jest-transform.storybook.now.sh

@edelgado
Copy link
Contributor

Can't wait for this either, thanks! Maybe remove the dependency on deasync-promise as explained in #7330 (comment) ? Thanks!

@ndelangen
Copy link
Member Author

Would you like to contribute to this PR @edelgado?

@edelgado
Copy link
Contributor

@ndelangen sure thing! Thanks for the access, will submit a commit soon.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

NICE!! looking very clean. would it be possible to update the documentation as well?

@edelgado
Copy link
Contributor

@shilman Absolutely! Added to both add-ons to make it easier on the users.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Thanks @edelgado, this looks great. Last request: can we add this to our monorepo somehow to test it?

@edelgado
Copy link
Contributor

edelgado commented Oct 4, 2019

Thanks @edelgado, this looks great. Last request: can we add this to our monorepo somehow to test it?

Hi @shilman, not sure what you mean by this. First time contributing so I'm not familiar with that task. Could you point me in the right direction please?

@shilman shilman assigned shilman and unassigned ndelangen Oct 7, 2019
@ndelangen
Copy link
Member Author

@shilman what do you require?

@shilman
Copy link
Member

shilman commented Oct 7, 2019

@ndelangen working with @Hypnosphi to get this mergeable

@Hypnosphi
Copy link
Member

Hypnosphi commented Oct 9, 2019

@shilman looks like you're adding the transform twice, as it's already present in config.transform.

In my case, right before your commit, the html snapshotting went OK, but storyshots-integrity complained, probably because its algorythm doesn't know anything about mdx extension yet

The other failures (vue, angular) were the same as you see now on CI

UPD: I re-ran the CI tests on my commit, so that you could see the original "Storyshots Integrity" error
https://circleci.com/gh/storybookjs/storybook/163146 -> Cmd+F for "Storyshots Integrity"

@shilman
Copy link
Member

shilman commented Oct 10, 2019

@Hypnosphi I fixed the integrity error in my latest commit. However, I'm not sure what's going on with Vue/Angular. Do you have any ideas?

@Hypnosphi
Copy link
Member

@shilman not really

@shilman
Copy link
Member

shilman commented Oct 24, 2019

@ndelangen and I were able to get angular working. Vue is not working due to a babel issue, and I've temporarily disabled it to move this forward. I'll follow up with @Aaron-Pool to see if we can get that working. Pretty sure it's more of a Vue/Babel issue than a Jest/MDX one.

@shilman shilman changed the title Addon-docs: Add jest-transform.js to convert .mdx => .js for storyshots Addon-docs: Add Jest MDX transform for storyshots Oct 24, 2019
@shilman shilman changed the title Addon-docs: Add Jest MDX transform for storyshots Addon-docs: Jest MDX transform for storyshots Oct 24, 2019
@shilman shilman merged commit d57dd04 into next Oct 24, 2019
@shilman shilman deleted the jest-transform branch October 24, 2019 13:42
```json
{
"transform": {
"^.+\\.[tj]sx?$": "babel-jest",
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there a pipe symbol missing: [t|j] ? As here: https://github.com/facebook/jest/tree/master/packages/babel-jest#setup

```json
{
"transform": {
"^.+\\.[tj]sx?$": "babel-jest",
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the pipe symbol necessary? [t|j]

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