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 named export for example components #1380

Merged
merged 5 commits into from
Jun 11, 2019

Conversation

mendrew
Copy link
Contributor

@mendrew mendrew commented Jun 5, 2019

iss: #1281

Updated examples-loader to import component with named export
Named export of Basic button is now working

I tried suggestion const name = name$0[name] || name$0.default || name$0;
CounterButton component is now falling with an error:

Invariant Violation: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: object.
Check the render method of `FunctionComponentWrapper`.

CoutnerButtons is falling

Placeholder from my previous report is now working.

iss: styleguidist#1281

Updated examples-loader to import component with named export
Named export of Basic button is not working, but CounterButton is
now falling with an error:

```
Invariant Violation: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: object.
Check the render method of `FunctionComponentWrapper`.

```
@mendrew
Copy link
Contributor Author

mendrew commented Jun 6, 2019

Ok, I finally get the idea why with this fix we have problems with other components from example.
The reason is simple but I've been debugging a lot till I get it.

First example: CounterButton.

Markdown code for CounterButton looks like this

import Button from '../Button'
let ref
;<div>
  <CounterButton ref={r => (ref = r)} />
  <Button size="small" onClick={() => ref.set(0)}>
    Reset
  </Button>
</div>

The error is about this import import Button from '../Button'. Button is no longer has default export in my PR and Button should be imported using named import.

Second example: was tricked by it

import React from 'react'
import Button from 'rsg-example/components/Button'
import Placeholder from 'rsg-example/components/Placeholder'
;<Button>
  <Placeholder />
</Button>

In this case I had error with import Button from 'rsg-example/components/Button', I thought it is absolutely different button, but then I looked at the styleguidist config and found this
https://github.com/mendrew/react-styleguidist/blob/1ed431d693af196c8c4d8f84839c60ac06274f2b/examples/basic/styleguide.config.js#L8

It's the same button component, and we should import it in the same way with named import.

@mendrew
Copy link
Contributor Author

mendrew commented Jun 6, 2019

I thinks that ast bulder code update const name = name$0[name] || name$0.default || name$0; is enough.

I'm going to remove WIP code and update tests.

mendrew added 2 commits June 7, 2019 00:52
Added new version of expected require for named export case
@mendrew mendrew changed the title WIP: Debug named export error for example components Fix named export for example components Jun 6, 2019
@mendrew
Copy link
Contributor Author

mendrew commented Jun 6, 2019

I've finally updated the PR. It's ready for review.

I'm wondering does it make sense to add named export to some basic example code, just to have this case in examples?

@sapegin
Copy link
Member

sapegin commented Jun 7, 2019

Haha, this all sounds very familiar! ;-) And thanks for fixing so many things in Styleguidist!

I'm wondering does it make sense to add named export to some basic example code, just to have this case in examples?

Yeah, that would make sense — tests aren't always enough to make sure things are still working.

Had to break "prefer default export" rule
@mendrew
Copy link
Contributor Author

mendrew commented Jun 9, 2019

Haha, this all sounds very familiar! ;-) And thanks for fixing so many things in Styleguidist!

You are welcome! I found the code pretty interesting and clear. So it's a kind of pleasure to debug 🙂. I haven't used it yet. But definitely will. And all this design system area looks inspiring.

Yeah, that would make sense — tests aren't always enough to make sure things are still working.

I've updated the PR. Added such kind of import into RandomButton, so it likely hurt anyone who uses somehow example/basic components.
Did update for RadmonButton markdown to be explicit in a way we import this button.

Any other suggestions? @sapegin

@codecov-io
Copy link

codecov-io commented Jun 9, 2019

Codecov Report

Merging #1380 into master will not change coverage.
The diff coverage is n/a.

Impacted Files Coverage Δ
src/loaders/examples-loader.js 100% <ø> (ø) ⬆️
src/scripts/schemas/config.js 89.55% <0%> (ø) ⬆️

@sapegin sapegin merged commit 5ff72d8 into styleguidist:master Jun 11, 2019
@sapegin
Copy link
Member

sapegin commented Jun 11, 2019

Thanks! 🦄

@styleguidist-bot
Copy link
Collaborator

🎉 This PR is included in version 9.1.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants